Skip to content

Added 80 column fix from Sorcha code - #442

Merged
matthewholman merged 2 commits into
mainfrom
fix-80char-limit-on-spice
Jul 21, 2026
Merged

Added 80 column fix from Sorcha code#442
matthewholman merged 2 commits into
mainfrom
fix-80char-limit-on-spice

Conversation

@Little-Ryugu

Copy link
Copy Markdown
Collaborator

Added _split_kernel_path_str and its unit test from Sorcha to avoid 80 column bug

Fixes #433 .

Describe your changes.

Review Checklist for Source Code Changes

  • Does pip install still work?
  • Have you written a unit test for any new functions?
  • Do all the units tests run successfully?
  • Does Layup run successfully on a test set of input files/databases?
  • Have you used black on the files you have updated to confirm python programming style guide enforcement?

Added _split_kernel_path_str and its unit test from Sorcha to avoid 80 column bug
@matthewholman

Copy link
Copy Markdown
Collaborator

Nice work, Ryan — tested it end-to-end and it fixes the bug. A long (~165-char) cache path reproduced the TYPEMISMATCH on main. Confirmed your change lets a kernel actually load through the generated meta_kernel.txt. The comma in +',\n' is the real fix.

One non-blocking note: the unit test checks the split string but never calls furnsh, so it wouldn't catch a break in the actual SPICE load. Here's a short integration test that guards the real behavior — builds a meta_kernel under a deliberately deep path and loads a kernel through it:

import os
import spiceypy
from layup.utilities.bootstrap_utilties.create_meta_kernel import _split_kernel_path_str


def test_meta_kernel_loads_under_long_path(tmp_path):
    # A cache path long enough to trip the SPICE 80-char string limit.
    deep = tmp_path
    while len(str(deep)) < 160:
        deep = deep / "12345678"
    deep.mkdir(parents=True, exist_ok=True)

    # A tiny kernel we can check actually loaded.
    (deep / "tiny.tls").write_text(
        "\\begindata\nDELTET/DELTA_T_A = 99.5\n\\begintext\n"
    )

    mk = deep / "meta_kernel.txt"
    mk.write_text(
        "\\begindata\n\n"
        f"PATH_VALUES = ('{_split_kernel_path_str(str(deep))}')\n\n"
        "PATH_SYMBOLS = ('A')\n\n"
        "KERNELS_TO_LOAD = ( '$A/tiny.tls' )\n\n"
        "\\begintext\n"
    )

    spiceypy.kclear()
    try:
        spiceypy.furnsh(str(mk))                     # would raise TYPEMISMATCH before the fix
        assert spiceypy.gdpool("DELTET/DELTA_T_A", 0, 1)[0] == 99.5
    finally:
        spiceypy.kclear()

(Also, docstring says default 79 but it's split=77.) Approve after that — thanks for the quick turnaround!

@Little-Ryugu

Copy link
Copy Markdown
Collaborator Author

No worries. I fixed the docdtring for sorcha but forgot layup oops. Thats a very good point with the unit test, it caused us to miss the bug in Sorcha. I'll add that test to both repos too thank you!!

@matthewholman matthewholman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@matthewholman
matthewholman merged commit 4c7eef9 into main Jul 21, 2026
10 of 11 checks passed
@matthewholman
matthewholman deleted the fix-80char-limit-on-spice branch July 21, 2026 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

orbitfit/convert/predict fail with SPICE TYPEMISMATCH when the data-cache path is long

2 participants