KPLO ShadowCam: subsample the ephemeris by default (as for Chandrayaan-2)#719
Conversation
ShadowCam images are ~85K lines; per-line ephemeris produces ~19MB ISDs that are too large to load efficiently into the CSM. Override ephemeris_time() to default reduction=linear (overridable via props), exactly as the Chandrayaan-2 driver does for its ~200K-line images. Full-cube ISD drops 19MB -> 2.1MB with no loss of camera accuracy (cam_test vs ISIS unchanged at ~0.001px). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Unit test verifies the ShadowCam driver defaults the ephemeris reduction to linear (subsampled), and overrides an explicit none. Base ephemeris_time is mocked so no kernels are needed. Plus a CHANGELOG entry for the change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I think we need to at least notify or output something that reduction==None is not supported for shadowcam. This would help with coneyance. We should probably add the same to the CH-2 drivers that are bypassing reduction==None. |
Per review feedback on PR 719: when the ShadowCam and Chandrayaan-2
TMC-2/OHRC drivers override the reduction (the default, and an explicit
--reduction none, which cannot be respected for these large-strip
sensors), print a notice to stderr so the subsampling is conveyed to the
user. Update the changelog accordingly.
ALE ignores Python warnings globally (warnings.filterwarnings("ignore")
in ale/__init__.py) and defaults the logger to ERROR, so warnings.warn
and logger.warning are both swallowed by default; a plain stderr print
is used so the notice is always visible.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This is Claude (AI assistant), working with Oleg on this PR. Good call on conveyance — done, and extended to Chandrayaan-2 as you suggested. When the ShadowCam driver defaults the reduction (both the no-flag default and an explicit A note on how the notice is emitted, since it took some figuring out. The natural choices both turn out to be invisible by default in ALE: Verified on real data. Each default run prints the notice and produces the subsampled ISD. "Full" is one ephemeris sample per line (
The reduction is a straight ~10x: size scales with lines / sample_rate. TMC-2's default ISD is still 7.3 MB only because its strip is ~190k lines; Also checked that an explicit |
| print("Chandrayaan-2 TMC-2: per-line ephemeris ISDs are very " | ||
| "large. Defaulting reduction to 'linear'. Pass --reduction " | ||
| "linear --ephem_sample_rate N to control the sampling. An " | ||
| "explicit --reduction none cannot be respected for this " | ||
| "sensor.", file=sys.stderr) |
…print Route the KPLO ShadowCam and Chandrayaan-2 TMC-2/OHRC reduction-defaulting notices through the ale package logger, as requested in review, instead of print to stderr. Update the changelog wording to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Done. Switched all three notices (KPLO ShadowCam and Chandrayaan-2 TMC-2/OHRC) from print-to-stderr over to logger.info via the ale package logger, and dropped the now-unused sys import. Updated the changelog wording to match. CI is green. One thing to flag: since the ale logger defaults to level ERROR, this notice is now silent unless the user runs verbose (isd_generate -v or ALE_LOG_LEVEL=INFO). So the "I defaulted reduction to linear, an explicit --reduction none cannot be respected for this sensor" message will not be seen by default anymore. That is fine with us, and it matches the direction in #717. If you later want such notices visible by default, that is a logging-level decision on your end rather than something this PR should force. As far as we are concerned this is implemented as requested and we hope it can be merged. Standard disclaimer: this comment and the commit were done by an automated assistant (Claude). |
| if not hasattr(self, "_ephemeris_time"): | ||
| reduction = self._props.get('reduction', 'none').lower() | ||
| if (reduction == 'none'): | ||
| logger.info("Chandrayaan-2 OHRC: per-line ephemeris ISDs are " |
There was a problem hiding this comment.
I think a warning is more appropriate
… notice Per review feedback: a warning is more appropriate than an info-level message when the driver overrides the requested reduction. Applies to the KPLO ShadowCam and Chandrayaan-2 TMC-2/OHRC drivers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Replaced the info-level reduction notice with a warning, as suggested. The KPLO ShadowCam and the Chandrayaan-2 TMC-2 and OHRC drivers now emit it via logger.warning. The Linux, macOS, and docs checks pass. The Windows jobs failed before any tests ran, on an unrelated conda environment setup and CMake configuration step, not on this change. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Kelvinrr
left a comment
There was a problem hiding this comment.
test failure seems unrelated
The KPLO ShadowCam driver writes one ephemeris and rotation sample per image line. ShadowCam strips are long: a typical strip is about 85,000 lines (the two test scenes are 3144 x 84992), so the CSM ISD comes out around 19 to 20 MB. That is slow to write and to load, and finer than the smoothly varying spacecraft trajectory needs.
The Chandrayaan-2 driver (TMC-2 and OHRC, images near 200,000 lines) handles this by overriding
ephemeris_time()to default the reduction to "linear", so the ISD is subsampled unless the caller asks otherwise. It may make sense to do the same in the KPLO ShadowCam driver.This was checked on a stereo pair, M074289249SE and M074296291SE. Each ISD was generated at full resolution, and then reduced by a factor of 10 (about 19 MB down to about 2 MB). cam_test comparing the two cameras gave a maximum pixel difference of 1.05e-04 for M074289249SE and 1.08e-04 for M074296291SE:
Bundle adjustment and stereo on the same pair gave very close results at full resolution and reduced (the same convergence and outlier behavior, and DEMs within stereo noise).
This PR also adds a unit test,
tests/pytests/test_kplo_drivers.py::test_ephemeris_reduction_defaults_to_linear, which mocks the baseephemeris_timeand verifies the driver defaults the reduction to "linear" (and overrides an explicit "none"). It needs no kernels, matching the existing mocked driver tests. A CHANGELOG entry is included, and all CI checks pass.On the interaction with the
isd_generate --reductionoption: as with Chandrayaan-2, the driver only sets the default when the caller leaves--reductionat "none", so the default output is subsampled. An explicit--reduction noneis overridden for this dataset, and--reduction linear --ephem_sample_rate Nis still respected, so a caller can resample more or less if desired.Developed with AI (Claude) assistance.