-
Notifications
You must be signed in to change notification settings - Fork 3
CAL spectra : daily energy calibration #427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #427 +/- ##
==========================================
- Coverage 72.23% 68.14% -4.10%
==========================================
Files 78 78
Lines 8085 8296 +211
==========================================
- Hits 5840 5653 -187
- Misses 2245 2643 +398 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
samaloney
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The managers are all very similar I wonder if we shouldn't make a general manager and then have specific versions?
Also I think we should avoid adding more of the singleton pattern, I know I was the one who started with it but it really an anti-pattern in python in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a new CAL processing “level” focused on generating daily energy calibration products (migrating prior “L2 energy calibration” intent into CAL), and adds supporting calibration/config management utilities.
Changes:
- Add a new CAL
EnergyCalibrationproduct implementation and adjust FITS naming/writing/publishing to include CAL outputs. - Introduce ECC/ELUT manager utilities (plus tests) to support date-dependent calibration configuration and ELUT lookup.
- Re-enable parts of L2 processing and add scaffolding for L2 science spectrogram handling and extra FITS extensions.
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
stixcore/soop/manager.py |
Temporarily disables SOOP API fallback when the cached “all” file is missing. |
stixcore/products/product.py |
Adds loading of optional additional FITS extensions when reading products; adds L2Mixin.get_additional_extensions(). |
stixcore/products/level2/scienceL2.py |
New L2 spectrogram product scaffold with optional corrected data extension. |
stixcore/products/level2/quicklookL2.py |
Removes the L2 energy calibration product (moved to CAL). |
stixcore/products/level1/scienceL1.py |
Minor class metadata edit (processing version). |
stixcore/products/level0/scienceL0.py |
Copies optional additional-extension tables when splitting into file chunks. |
stixcore/products/__init__.py |
Exposes the new CAL energy calibration product and L2 science product(s). |
stixcore/products/CAL/energy.py |
New CAL energy calibration product that runs ECC and derives energy edges. |
stixcore/processing/publish.py |
Adds CAL to default published levels. |
stixcore/processing/pipeline_cron.py |
Re-enables L2 processing step (currently with a result-reset issue). |
stixcore/processing/FLtoL3.py |
New (currently broken) flare-list to L3 processing scaffold. |
stixcore/io/product_processors/fits/processors.py |
Adjusts filename generation for cal type and allows writing CAL via L2 processor. |
stixcore/io/ProcessingHistoryStorage.py |
Comments out a future DB migration section. |
stixcore/ecc/manager.py |
New ECC configuration manager with temporary context creation. |
stixcore/ecc/tests/test_ecc_manager.py |
Adds ECC manager tests. |
stixcore/calibration/elut_manager.py |
New ELUT manager wrapper around stixpy readers. |
stixcore/calibration/tests/test_elut_manager.py |
Adds ELUT manager tests (currently assumes missing repo data). |
stixcore/calibration/ecc_post_fit.py |
Adds ECC post-fit logic used by CAL energy calibration. |
stixcore/data/test.py |
Adds test_data.ecc path. |
stixcore/data/stixcore.ini |
Adds [ECC] ecc_path config key. |
stixcore/__init__.py |
Attempts to guard __version_conf__ import (currently incorrect). |
pyproject.toml |
Adds lmfit==1.3.4 dependency. |
docs/pipelineconfiguration.rst |
Removes pipeline monitor documentation section. |
.github/workflows/end2end_run.yml |
Whitespace cleanup. |
Comments suppressed due to low confidence (1)
stixcore/products/level1/scienceL1.py:154
PRODUCT_PROCESSING_VERSIONis now defined twice in this class. The duplicate assignment is redundant and makes it easy for the two values to diverge later; keep only one definition.
PRODUCT_PROCESSING_VERSION = 4
NAME = "xray-spec"
PRODUCT_PROCESSING_VERSION = 4
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add support for trigger scaling in compression mode S=0,k=0,M=7 (i4Ds#377) * Unscale data when compressed with mode S=0,k=0,M=7 * QL - LC, BKG, SPECTRA * BSD - CPD, SCPD, VIS and SPEC bump all product versions to 2 (i4Ds#378) * bump all product versions to 2 * roleout IDB version 2.26.27 * fix test with new version number in mock object * fix test * fix for latest IDB as fallback Fix duplicated hk time bin bug Ddpdupdates2 (i4Ds#380) * bump all product versions to 2 * prep for L2 ddpd * integrate IDB release 2.26.38 * update for common CONF submodule * fir text for latest IDB to 2.26.38 Config fix (i4Ds#381) * bump all product versions to 2 * fix module path Fix bugs found when processing new scaled trigger data. (i4Ds#382) * Fix bug processing new scaled trigger data. * Allow for QL file to unscale by packet * Fix bug that create extra (replica) triggers in Spectrogram Publish_L2 / RID LUT Manager (i4Ds#385) * bump all product versions to 2 * RID LUT data manager update and use in processing for trigger descaling * Update stixcore/io/RidLutManager.py * fixed API mock via static files on pup99 --------- Co-authored-by: Shane Maloney <maloneys@tcd.ie> Add warning if trigger scaling changes in request (i4Ds#388) End2end rid lut manager (i4Ds#390) * bump all product versions to 2 * add copy and parent option * fix missing init of singleton bump all product versions to 2 (i4Ds#389) Bump conf version (i4Ds#391) * bump all product versions to 2 * bump common version Flexible way to fill DATAMAX/MIN BUNIT EXPOSURE in FITS header keywords (i4Ds#393) * bump all product versions to 2 * add properties for dedicated fits header keywords with class inhe. / override * add test and minor fixes after review * fix XPOSURE/XPOMAX header error * fix tests for dmax... * relax end2end fitsdiff * less tolerance in end2end test Solo anc stix ephemeris (i4Ds#399) * bump all product versions to 2 * add more FITS header keywords * fix for ANC also get fits updated headers while publishing Upgrade_watchdog (i4Ds#401) * bump all product versions to 2 * bump version * fix version label * pin common CONF version for end2end test * ignore VERS_CFG in end2end testing * add test for TM folder observing * bump watchdog==6.0.0 * integrate review fixes Update docks (i4Ds#409) * bump all product versions to 2 * update documentation * add doc for end to end tetsing * fix format * add doc for manual (re)processing of fits products E2e fix sym add (i4Ds#412) * bump all product versions to 2 * dep np.bool8 * disable test Get rid reason/scaling lookup error handling (i4Ds#411) * bump all product versions to 2 * add global error detection in pipeline logger (not just main process) * add exception handling for get_reason daily processing pipeline (i4Ds#405) * bump all product versions to 2 * initial * daily processing task * add singleStep * move ANC-aspect from pipeline to daily_pipeline * removed requirement for the moment * fix i4Ds#283 and option to process only latest fits file versions * update ddpd scripts * fix circular imports * test git dep * skio test on win * skip test on win/mac * Update stixcore/io/FlareListManager.py Co-authored-by: Shane Maloney <maloneys@tcd.ie> * fix format error --------- Co-authored-by: Shane Maloney <maloneys@tcd.ie> Add Zenodo file (i4Ds#413) * Add Zenodo author information Update high resolution transmission to match IDL defaults (i4Ds#414) Pin major and minor versions of some packages (i4Ds#415) V1.5.1_fixes (i4Ds#420) * bump all product versions to 2 * fix i4Ds#417 * fix i4Ds#418 * daily report log files could have different names if the scripts run over multiple days * fix i4Ds#419 * end2end test: better report if no corresponding file found * rewrite pipeline to cronjob based approach i4Ds#422 * cleanup before merge Fix bug in spectrogram time bin calculations (i4Ds#423) * Only use the closing time offset (NIX00269) for the last science substrure Update .zenodo.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated 23 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
stixcore/calibration/ecc_post_fit.py
Outdated
| PIXELs = LARGEs + SMALLs | ||
|
|
||
| # Prep Pandas DataFrame | ||
| df = pd.DataFrame() # dict will be define later during concactenation with appropriated keys |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: "concactenation" should be "concatenation".
| df = pd.DataFrame() # dict will be define later during concactenation with appropriated keys | |
| df = pd.DataFrame() # dict will be define later during concatenation with appropriated keys |
|
|
||
| # Create singleton instance | ||
| if "pytest" in sys.modules: | ||
| ELUTManager.instance = ELUTManager(test_data.elut if hasattr(test_data, "elut") else None) |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The singleton instance initialization checks for test_data.elut but this attribute doesn't exist in the TestData class (see stixcore/data/test.py). This will always evaluate to None in pytest runs, which may cause issues if tests expect ELUT data to be available. Consider adding an ELUTTestData class to test.py or removing the hasattr check.
| ELUTManager.instance = ELUTManager(test_data.elut if hasattr(test_data, "elut") else None) | |
| ELUTManager.instance = ELUTManager() |
| def is_datasource_for(cls, *, service_type, service_subtype, ssid, **kwargs): | ||
| return ( | ||
| kwargs["level"] in [EnergyCalibration.LEVEL, "L2"] | ||
| and service_type == 21 | ||
| and service_subtype == 6 | ||
| and ssid == 41 | ||
| ) |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The is_datasource_for method accepts both "CAL" and "L2" levels. While this might be intentional for migration purposes (as mentioned in the PR description), it could lead to ambiguity. Consider whether this is truly necessary or if it should only accept "CAL" level. If both are required, document why in a comment.
| bash_script = f"""#!/bin/bash | ||
| cd {ecc_run_context_path} | ||
|
|
||
| {ecc_install_path}/Bkg | ||
| {ecc_install_path}/ECC --f_obs "{spec_filename}" | ||
| {ecc_install_path}/STX_Calib spec_all.fits ECC_para.fits[1] | ||
| """ |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential command injection vulnerability: spec_filename is used in the bash script after simple string replacement but is still derived from FITS header data. Although there's a regex validation (line 170), the filename is interpolated into the bash script at line 195 inside double quotes, which could allow command injection if the filename contains special characters like backticks or dollar signs that bypass the regex. Consider using subprocess.run with a list of arguments instead of shell=True, or use shlex.quote() to properly escape the filename before interpolation.
|
|
||
|
|
||
| def poly(x, degree, slope, intercept): | ||
| """a line""" |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring for the poly function says "a line" but it actually represents a quadratic polynomial (degree * x^2 + slope * x + intercept). Update the docstring to "a quadratic polynomial" for accuracy.
| """a line""" | |
| """a quadratic polynomial""" |
| def Read_fits_STIX_One_Pixel(data, PIX, DETECTOR_ID, Nbin=2024, NRebin=1, NSigma=1): | ||
| Pix = PIX | ||
| Pix = Pix + (DETECTOR_ID - 1) * 12 | ||
| obs = [0] * Nbin |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assignment to 'obs' is unnecessary as it is redefined before this value is used.
| obs = [0] * Nbin |
stixcore/calibration/ecc_post_fit.py
Outdated
| # Prep Pandas DataFrame | ||
| df = pd.DataFrame() # dict will be define later during concactenation with appropriated keys |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assignment to 'df' is unnecessary as it is redefined before this value is used.
| # Prep Pandas DataFrame | |
| df = pd.DataFrame() # dict will be define later during concactenation with appropriated keys | |
| # Prep Pandas DataFrame: dict will be defined later during concatenation with appropriate keys |
| nbin = Nbin | ||
| Nbin = [Nbin / NRebin] |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable Nbin is not used.
| nbin = Nbin | |
| Nbin = [Nbin / NRebin] | |
| # determine number of bins to use, taking into account any requested rebinning | |
| nbin = int(Nbin / NRebin) |
| accumulator = [] | ||
|
|
||
| # Proceed to fit individually each pixel spectrum in the list of files | ||
| for DET in DETs: |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This for-loop may attempt to iterate over a non-iterable instance of class int.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
this PR started a while ago as first version of general L2 products as QL calibration spectra and sci-spectra as L2
then it moved its goal to daily energy calibration files that have become an own "level": CAL
so some code parts for L2 files are commented out to be eneabled later again with our first L2 products