Support ingesting ACA L0 image telemetry into cheta archive#291
Conversation
This is unrelated but modern editors really want to do this.
- Temperatures (K, DEGF, DEGC) - Backgrounds (DN)
This reverts commit 7ca1bf4.
jeanconn
left a comment
jeanconn
left a comment
There was a problem hiding this comment.
In addition to the TODO items in the description it seems like this should have at least a couple of new unit tests.
|
@jeanconn - I added a unit test and wrote a promotion plan. AFAIK this is fully ready now. |
taldcroft
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds support for ingesting ACA Level-0 (L0) image telemetry into the cheta archive, sourcing the raw FITS files from the mica ACA L0 archive (per slot) and validating that cheta’s ingested values match mica’s L0 content. It also updates tooling/config to support the new content type and modernizes a few maintenance notes and lint settings.
Changes:
- Add new ACA L0 content types (ACA0–ACA7) and an ACA-specific FITS retrieval path that copies files from the mica archive instead of using arc5gl.
- Add an ACA L0 converter that normalizes per-slot columns and stores the 8×8 image telemetry in a compact archive-friendly representation.
- Add validation artifacts (notebook + pytest coverage) and update ruff config / notes / sync behavior for content naming.
Reviewed changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| validate/PR291-aca-level0-telemetry.ipynb | Notebook demonstrating ACA L0 ingest validity vs mica and unit handling. |
| setup.py | Improves error messaging when ska_helpers is missing during setup. |
| ruff.toml | Refines exclude list for linting specific dev/legacy files. |
| ruff-base.toml | Updates Ruff target Python version and excludes .eggs. |
| NOTES/NOTES.add_derived | Updates legacy instructions to modern python -m cheta... invocation. |
| NOTES/NOTES.add_content | Adds a modern-reference note and updates archive update command usage. |
| cheta/update_server_sync.py | Normalizes content names to lowercase when updating sync repos. |
| cheta/update_archive.py | Implements ACA FITS acquisition via mica, adjusts stats handling, and updates ingest behavior for ACA gaps and N-D concatenation. |
| cheta/tests/test_fetch.py | Adds tests validating cheta ACA L0 content against mica L0 content. |
| cheta/filetypes.dat | Registers ACA0–ACA7 as new L0 content types. |
| cheta/derived/base.py | Adds a brief docstring to DerivedParameter.__call__. |
| cheta/converters.py | Adds ACA L0 conversion logic and type annotations for FITS conversion entrypoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Predeclare numpy arrays of correct type and sufficient size for accumulating results. | ||
| out = OrderedDict() | ||
| out = {} | ||
| out["index"] = np.ndarray((n_out,), dtype=np.int32) | ||
| out["n"] = np.ndarray((n_out,), dtype=np.int32) | ||
| out["val"] = np.ndarray((n_out,), dtype=msid_dtype) | ||
| if msid_is_1d: | ||
| out["val"] = np.ndarray((n_out,), dtype=msid_dtype) |
There was a problem hiding this comment.
This is a good point that I had noticed earlier but forgot to fix. There is little practical value in the representative val sample and regenerating the archive would be a large effort. But I think one safe option is to change __len__ to use len(self.times), which is always defined.
| out["index"][i] = index | ||
| out["n"][i] = n_vals | ||
| out["val"][i] = vals[n_vals // 2] | ||
| if msid_is_numeric: | ||
| if msid_is_1d: | ||
| out["val"][i] = vals[n_vals // 2] | ||
| if msid_is_numeric and msid_is_1d: |
There was a problem hiding this comment.
Fair point, but as before the out["val"] is not that useful.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ametrizing on slot.
self.vals might not exist, e.g. for stats on a 2-d MSID.
There was a problem hiding this comment.
These are Claudio's findings.
- I verified this one
- No idea where it got the idea that "thousands" of files would pile up after an outage. I'm guessing this is not an issue, but you can decide.
- this is a code-duplication comment. Do we care?
- not sure I understood this one
- he's got a point
- also got a point
- 50/32 is everywhere in our codebase. I guess we ignore this one.
Findings (7, most-severe first)
1. Fetching 5min/daily stats for any ACA{n}_IMGTLM MSID crashes (correctness, confirmed)
calc_stats_vals stores only index+n for N-d MSIDs, but the stats loop at update_archive.py:300-311 still runs for IMGTLM, writing a real stats file. On read, fetch.py:_get_stat_data only sets self.vals when a means/val column exists — it doesn't here — so len(dat)/repr/plot/interpolation hit fetch.py:620 return len(self.vals) → AttributeError. Propagates to synced clients via update_server_sync.
2. get_archive_files_aca ignores opt.max_arch_files (efficiency/memory)
update_archive.py:1406 returns every mica file in [datestart, datestop] uncapped, unlike the other two return paths (1325 slices [:opt.max_arch_files]; the arc5gl path is chunked with the comment "Don't allow arbitrary arch files at once because of memory issues"). On --create or after an outage, thousands of 8×8 image files get copied and held in dats for a single np.concatenate → OOM risk.
3. aca_converter re-implements generic_converter's prefix + quality tail (reuse)
converters.py:237-244 rebuilds the "all-False QUALITY of width len(colnames)" array and the "prefix every column except TIME/QUALITY" loop that generic_converter (converters.py:63-78) already provides. Two copies of the quality-width/prefix convention to keep in sync.
4. Three ACA special-cases keyed on two different attributes; IMGTLM kept by a fragile regex (altitude)
update_archive.py:1138 re.match(r"ACA\d_IMGTLM", colname) is the only thing keeping the N-d column in colnames. A second N-d ACA column or a rename is silently dropped (info-log only). Combined with the instrum=="ACA" branches for max_gap and file-fetch, ACA support is spread across 3 functions keyed on colname-regex vs instrum-string.
5. Eight hand-numbered aca0..aca7 lambdas (simplification)
converters.py:247-254. The slot is already in the content name (ACA0..ACA7), and convert() dispatches via getattr(MODULE, content.lower()). A single converter that parses int(content[-1]) removes the boilerplate and the mis-numbering risk (e.g. aca6 = lambda dat: aca_converter(dat, slot=5) would silently mislabel a slot). Note: E731 is ignored in ruff-base.toml, so this is not a lint violation.
6. test_aca_l0_content rebuilds all 8 slots on every parametrization (efficiency, test)
test_fetch.py:771-773 builds dat_micas for range(8) inside a test parametrized over 8 slots → 64 get_slot_data calls for 8 slots of coverage. The comprehension's slot also shadows the parametrized slot, making the intent ambiguous.
7. The packet↔DN constants 50/32 are hard-coded inline in both converter and test (reuse)
converters.py:231 and test_fetch.py:786 each re-encode the transform. Because the test hard-codes the exact inverse, it cannot catch a converter error in the same constants — both would have to be edited together, and a shared/named helper (ideally beside mica's forward decom) would prevent drift.
|
For 1), I think the stat handling does still need work and a trivial test. |
|
Regarding 4), what is "altitude"? |
Ha! While you were asking, I was asking the same question, and this was the answer: "Altitude" is about whether a change is made at the right level/depth in the code — the metaphor is how high up you're operating: down in the weeds patching one specific case, vs. up at the level of the general mechanism. It comes from the review's angles. The question it asks: is this fix addressing the underlying thing, or is it a special case bolted onto shared infrastructure because the real fix would've been deeper? A pile of narrow special-cases is the classic "wrong altitude" smell — each one works, but the general mechanism that should have absorbed them didn't, so knowledge gets scattered and the next change has to touch all of them. Finding #4 in your review is the altitude one. ACA support is added as three separate special-cases in three different functions, keyed on two different things:
The "too low altitude" critique: instead of ACA-ness being a property the system reads (e.g. a flag on the filetype, or the converter declaring "this content has N-d columns"), it's re-detected ad hoc in each spot — one place by matching a colname string, others by matching the instrument string. So enabling/disabling/renaming means hunting down all three, and a second N-d column silently falls through the regex. A "higher altitude" fix would generalize the underlying mechanism once so those special-cases disappear. Contrast: the actual N-d storage support in that same PR ( |
|
@javierggt - Sorry, for some reason I had two commits locally from a few days ago that I had not pushed. Usually VS code shows those commits with a button to push / sync, but not this morning. Anyway, I just pushed. I think they fix (1) and (6) (more or less). Basically the |
With 4612ec3 this is no longer completely broken. But the real point is that I made a deliberate decision to not support statistics for Upshot: we don't care and it doesn't even matter if the stat value for |
This is not an issue in practice. The initial |
This does sound like a code duplication comment, but I'm not going to fix it. The existing code works and is tested. Refactoring to avoid duplication is definitely not worth the effort here. |
I think this is arguing to introduce some abstractions at a higher level to avoid special-case code in low-level functions. I agree with the concept in principle, but in practice here it is just not worth the effort for this mostly-legacy and highly-specialized processing code. |
This just seems completely off base and I don't understand it. The new code in this PR works within the expected API of this module.
That's just silly, the correct numbering is evident by inspection. |
Valid point and fixed. |
As noted earlier, those magic numbers are fixed in the ACA spec and won't ever change. Hardcoding like this is consistent with practice in the rest of our codebase. |
|
@javierggt @jeanconn - I think I have addressed all the review comments thus far. |
|
I do think that some independent functional testing of the cheta image telemetry would be valuable. |
|
"I do think that some independent functional testing of the cheta image telemetry would be valuable." What did you have in mind? I was actually thinking that the chandra_aca kinda does some of the actual user-related testing. |
|
I just meant that somebody besides me pokes around at the data and tries using the functions here. No big agenda, but just actually use it. |
|
It is still a little weird that "Fetching 5min/daily stats for any ACA{n}_IMGTLM MSID crashes (correctness, confirmed)" gives us an Msid with times but no vals, but if we're comfortable with that, it seems passable to me. |
|
Should this include a list of the common level0 columns in the fetch pseudo-msids docs page (or do we consider them not-pseudo-msids)? |
|
And in the testing "imgraw = aca_imgtlm.vals * (aca_imgscale.vals[:, None, None] / 32.0) - 50.0" suggested to me that maybe the 8 imgraw msids should be computed msids in cheta, but probably the chandra_aca PR fills in that gap. |
jeanconn
left a comment
jeanconn
left a comment
There was a problem hiding this comment.
I'm satisfied in our discussion that we're going to define the interface to this as the follow-on chandra_aca PR. So no need to extend docs etc.
Description
This is a long-time idea to ingest ACA L0 image telemetry into the cheta archive. Carrying out analysis for the Sampled Background patch has required heavy use of large volumes ACA pixel data, so this idea has become a higher priority.
The data are maintained as a separate content type by slot since each slot is independent in terms of time sampling. Only readouts with complete 8x8 images are ingested, starting from just before 2022:305. This coincides approximately with the start of always commanding 8x8 for all guide and acq stars.
This patch takes the L0 data directly from the mica ACA L0 archive instead of re-downloading it from the CXC archive. To keep cheta data most up to date it might be useful to have mica ACA L0 run before cheta, but mostly if the user cares about the most recent data they should be using MAUDE.
Follow-on PRs in
chandra_acamake this more convenient by reassembling the per-slot MSIDs into the usual table of L0 image rows.Promotion process
On HEAD as
aca, copy the data and set up links. This can be done prior to ska3-flight promotion since cheta only knows about content types in thefiletypes.datfile that is bundled in the installed package.On GRETA as
sot:Clean-up (for @taldcroft)
After ska3 release is completed and first automated updates are successful:
rm -rf /data/baffin/tom/data-cheta-aca0-pr291rm -rf /export/tom/cheta-aca0-pr291Archive creation on HEAD
This creates an archive that contains data starting at about 2022:305.
Create cheta_sync archive
Update cheta_sync archive
Interface impacts
Adds 280 new MSIDs to the cheta telemetry archive providing ACA image telemetry starting around 2022:305. The new data files currently require about 14 Gb of disk space.
Testing
Unit tests
Independent check of unit tests by Jean - just run with the new ACA0 data in my local archive
Functional tests
Test cheta_sync locally
This assumes the same setup as above, running from git repo.
Soak test in dev ska
Installed
ska3-acaandska3-flightto a newly created conda env in/export/tom/cheta-aca0-pr291:Test run by hand:
Install as a cron job running as aldcroft on kady and add to Ska cron jobs sheet:
Using and maintaining the ACA files prior to promotion
Install this version of cheta into a non-flight Ska environment with:
Update local Ska archive on a non-HEAD platform
Rsync the 14Gb of data files (dry-run first):
Make some necessary soft links:
Using on non-HEAD
It should just work in your dev Ska environment.
You can sync new data with:
Using on HEAD
On kady you can use a dev ska or the existing environment with this branch of cheta already installed:
This will only have access to the ACA L0 cheta telemetry, so you need to tell it to find all the rest as well: