Repository navigation
Conversation
Fits de/serialize hook for converting into fits serializeable icrs frame
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #458 +/- ##
==========================================
- Coverage 72.23% 68.27% -3.97%
==========================================
Files 78 80 +2
Lines 8085 9685 +1600
==========================================
+ Hits 5840 6612 +772
- Misses 2245 3073 +828 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
samaloney
left a comment
There was a problem hiding this comment.
I still need to check the code out and run it as hard to fully understand from just reading but looks good, left a few minor comments.
| CreateUtcColumn( | ||
| data, | ||
| [Time(d, format="isot", scale="utc") for d in mt["start_UTC"]], | ||
| "start_UTC", | ||
| description="start time of flare", | ||
| ) |
There was a problem hiding this comment.
Time(mt["start_UTC"]) does this work?
| # TODO redo | ||
| # if f_data_end > (Spice.instance.get_mk_date(meta_kernel_type="flown") - timedelta(hours=24)): | ||
| # return TestForProcessingResult.NotSuitable |
There was a problem hiding this comment.
we might have to adress that later this should ensure that we never process a monthly flare list that includes days we do not have flown kernels for. the rejection here will just postpone the processing day wise until the we have a suitable kernel
| super().on_deserialize(data, **kwargs) | ||
|
|
||
| @classmethod | ||
| def is_visible(cls, coord): |
There was a problem hiding this comment.
Need to careful about definitions here some of the problems should live on the examine in STIXpy as it depends - some sources can come from the corona
There was a problem hiding this comment.
Our is_visible was a modified copy of sunpy PR #7118, which has since been merged into sunpy as Helioprojective.is_visible(). I compared both on a grid of 24,480 positions (surface up to 3 R☉, observer Earth): identical results, and both match the exact geometry. So the flare list now calls sunpy's standard method directly, and nothing needs to move to stixpy. The definition question (coronal sources) is now covered by the column description, see the visible_from_earth thread.
rework documentation structure
samaloney
left a comment
There was a problem hiding this comment.
So the use case for some of the columns isn't clear to me, not to say there isn't a very good use case just I don't know about it.
On the imaging I think the integrated counts used to make the image could be useful to add.
The min and max exposure are of the selected CPD file used for imaging think just need to state there could be ohter files with different values.
Could we link all CPD file the overlap with the flare in a "related" files column?
|
|
||
| **review comment:** _(to be filled in via `notebooks/flarelist_sdcloc_review_comments.yaml`)_ | ||
|
|
||
| #### goes_min_class_est |
There was a problem hiding this comment.
I guess we need some documentation with the flare list so can describe where this stuff comes from e.g. the data centre not Muriel's work
There was a problem hiding this comment.
Agreed. Every column we take over unchanged from the STIX Data Center flare list now ends its description with "(provided by https://datacenter.stix.i4ds.net/ over stixdcpy)": flare_id, the start/peak/end times and duration, and all GOES_* columns. So it's clear in the file itself what comes from the data centre and what STIXCore computes. Each column's provenance is also listed in the column catalog, and we can add a short provenance section to the flarelist docs page as well.
|
|
||
| **review comment:** _(to be filled in via `notebooks/flarelist_sdcloc_review_comments.yaml`)_ | ||
|
|
||
| #### goes_max_class_est |
There was a problem hiding this comment.
So the classes are derived from the flux do we need both?
There was a problem hiding this comment.
i can not tell we will replace them until we have our own algo
|
|
||
| - **comment:** livetime- and area-corrected flux at the peak lightcurve bin (ct/s/keV/cm2, 5 energy channels) | ||
| - **unit:** ct / (keV s cm2) | ||
| - **example:** `[1.7785, 0.1944, 0.0736, 0.2886, 0.1244] ct / (keV s cm2)` |
There was a problem hiding this comment.
so this takes care of the det/pixel config so can directly compare counts event to event if the enable dets/pixels had changed?
There was a problem hiding this comment.
Yes for the flux columns: lc_peak_flux and bkg_spec_flux / bkg_spec_flux_ql are livetime-corrected and divided by the geometric area of the enabled detectors and pixels. Since this review they're also evaluated per time bin, so a configuration change within a day is picked up. The CPD background spectrum also ignores disabled detectors now (no counts, no exposure). The raw count columns (lc_peak, lc_bkg_peak, bkg_spec) aren't normalized and their descriptions now say so. One caveat: we only use the geometric area, not the grid transmission, so a different set of enabled detectors can still shift the flux slightly.
|
|
||
| **review comment:** _(to be filled in via `notebooks/flarelist_sdcloc_review_comments.yaml`)_ | ||
|
|
||
| #### lc_bkg_peak |
There was a problem hiding this comment.
which pixel in the bkg det are in this?
There was a problem hiding this comment.
All pixels of the background detector (detector 10, index 9), already summed on board. The QL background packet (NIX00278) only carries one sum per energy band and time bin.
|
|
||
| - **comment:** minimum CPD per-bin exposure ds (timedel) over the flare start..end window | ||
| - **unit:** ds | ||
| - **example:** `599 ds` |
There was a problem hiding this comment.
should also be in seconds
There was a problem hiding this comment.
Done, min_exposure and max_exposure are now stored in seconds (the CPD timedel is converted from ds). The catalog example still shows ds because it was generated from an older file; it will update with the next processing run.
|
|
||
| **review comment:** the actual time period (center time) on which the location is calculated is defined first in stixcore. The visibility code might (stixpy) might modify the time period to be used for the location calculation | ||
|
|
||
| #### location_duration |
There was a problem hiding this comment.
I'm not sure maybe it would be better to store the imaging timbering e.g start and end time used to make the image.
There was a problem hiding this comment.
Agreed. location_time_UTC and location_duration are replaced by location_start_UTC and location_end_UTC, the actual time range used to make the image (the visibilities' time range from stixpy). That matches start_UTC/end_UTC and avoids any doubt about whether the time is the start or the centre. The location coordinates refer to the centre of that range; the descriptions say so:
location_start_UTC: "start of the time range used for flare location estimation (imaging), UTC; the location columns refer to the centre of location_start_UTC..location_end_UTC"location_end_UTC: "end of the time range used for flare location estimation (imaging), UTC"
|
|
||
| - **comment:** maximum CPD per-bin exposure ds (timedel) over the flare start..end window | ||
| - **unit:** ds | ||
| - **example:** `792 ds` |
There was a problem hiding this comment.
same conversation to seconds
There was a problem hiding this comment.
Same as above, converted to seconds.
|
|
||
| **review comment:** _(to be filled in via `notebooks/flarelist_sdcloc_review_comments.yaml`)_ | ||
|
|
||
| ### 6. 1-AU normalization — [`add_distance_normalized_flux`](../stixcore/products/level3/flarelist.py#L94-L110) |
There was a problem hiding this comment.
Nice to have but since this should be easily calculated from two other columns is it worth it/useful I'm not sure need more user/external feedback
Agreed. The min_exposure / max_exposure descriptions now say they refer to the CPD file used for imaging (cpd_path), and that other CPD files covering the flare may have different time bins.
Added related_cpd_paths: all CPD files whose time range overlaps the flare start/end, ;-separated, in the same relative-path form as cpd_path. The first entry is always the file used for imaging (cpd_path); the rest follow the selection rank (peak inside, flare coverage, time resolution, energy bins). The min_exposure/max_exposure descriptions now point to it. |
No description provided.