Repository navigation
Fail ISD generation on null interior-orientation fields - #754
Conversation
SpiceQL can return null for required interior-orientation fields such as focal2pixel_lines, focal_length, or the detector center, for example when a web SpiceQL query fails. Driver.to_dict catches the error and returns None, so the ISD was written with a null field and isd_generate still reported success. That camera then crashes or behaves undefined downstream in usgscsm, such as an empty focal2pixel transform or a zero focal length. The formatter now checks these required fields and raises a clear error instead. Made with Claude AI assistance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| # these interior-orientation fields are required for any CSM model | ||
| required = ['focal_length', 'detector_center_line', 'detector_center_sample', | ||
| 'focal2pixel_lines', 'focal2pixel_samples'] | ||
| null_fields = [k for k in required if driver_data.get(k) is None] | ||
| if null_fields: | ||
| raise RuntimeError("Could not generate a valid ISD. Required field(s) " |
There was a problem hiding this comment.
probably need something better, this will most likely still be eaten up by "no such driver for label" error. Also, might as well check all required fields like positions and orientations.
Probably a bigger issue with error propagation in ALE.
There was a problem hiding this comment.
Something to consider is removing the to_dict entirely and revert back to directly accessing the properties on the driver.
We lose some of the async calls but gain appropriate error throws and failures when properties don't exist or can't be constructed
There was a problem hiding this comment.
Good catch, you were right it was being swallowed by the loader. Fixed so the invalid-ISD error now propagates to the user, with driver fallback preserved. Verified on the LRO NAC case.
There was a problem hiding this comment.
Agreed that removing to_dict for direct property access is cleaner, but it is a larger change. I kept this PR focused on the null check and making it propagate. Worth tracking the to_dict refactor as its own issue.
Addresses review feedback on the null-field check. The driver loader wrapped to_isd in a broad except that retried other drivers, so a raise in the formatter was swallowed and the user got a misleading "No viable Driver for Label" instead of the real cause. Introduce an InvalidISDError that the loader re-raises once a driver has already matched the label, so the clear message reaches the user. Also check the other required fields, the sensor positions, sun position, and frame chain (orientations), not only the interior orientation. Made with Claude AI assistance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The first attempt checked positions and orientations right after to_dict and re-raised InvalidISDError immediately. That broke the driver-selection loop, which tries several drivers for a label and relies on the wrong ones failing so it can fall through to the right one. Those wrong drivers legitimately return null for the frame chain and positions, so the eager check aborted before the correct driver was reached (CTX, MEX SRC load tests failed). Keep only the interior-orientation check, which runs late in to_isd after the frame chain and positions are already used, so a driver reaching it is the real one. The loader now remembers an InvalidISDError and keeps trying other drivers, raising it only if none succeed, so fallback still works and the clear message still reaches the user when the matched driver is the one that failed. Made with Claude AI assistance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Tests pass on all jobs after implementing the propagation fix Kelvin suggested. |
SpiceQL can return null for required interior-orientation fields such as focal2pixel_lines, focal_length, or the detector center, for example when a web SpiceQL query fails. Driver.to_dict catches that error and returns None, so the ISD was written with a null field and isd_generate still reported success. Accepting these silently produces a camera that crashes or behaves undefined downstream in usgscsm, such as an empty focal2pixel transform or a zero focal length divide. The formatter now checks these required fields and raises a clear error instead.
Done with Claude AI assistance.