Skip to content
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

Flexible way to fill DATAMAX/MIN BUNIT EXPOSURE in FITS header keywords #393

Merged
merged 15 commits into from
Oct 11, 2024

Conversation

nicHoch
Copy link
Collaborator

@nicHoch nicHoch commented May 13, 2024

so far only products with "count" data fill in this keywords in a usefull way. with this more flexible way of generating it should be much better to fill this keywords for deticated products

@nicHoch nicHoch requested a review from samaloney May 13, 2024 15:02
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 76.82119% with 35 lines in your changes missing coverage. Please review.

Project coverage is 75.82%. Comparing base (8146f04) to head (a4651d9).

Files with missing lines Patch % Lines
stixcore/products/level0/quicklookL0.py 68.57% 11 Missing ⚠️
stixcore/products/level1/quicklookL1.py 66.66% 8 Missing ⚠️
stixcore/products/level0/scienceL0.py 68.42% 6 Missing ⚠️
stixcore/products/level1/scienceL1.py 66.66% 6 Missing ⚠️
stixcore/products/level2/housekeepingL2.py 66.66% 3 Missing ⚠️
stixcore/processing/find.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
+ Coverage   75.75%   75.82%   +0.06%     
==========================================
  Files          68       68              
  Lines        7119     7250     +131     
==========================================
+ Hits         5393     5497     +104     
- Misses       1726     1753      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@samaloney samaloney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

In general always better to use to_value(<unit>) over value as if in different units value just give the raw number ignoring possible difference e.g unit prefix difference km v m.

stixcore/products/level2/housekeepingL2.py Outdated Show resolved Hide resolved
@@ -109,6 +130,19 @@ def __init__(self, *, service_type, service_subtype, ssid, control, data,
self.name = 'flareflag'
self.level = 'L1'

@property
def dmin(self):
return min([self.data['loc_y'].min(), self.data['loc_z'].min()])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is value or to_value() not needed here as below?

@@ -75,6 +75,21 @@ def __init__(self, *, service_type, service_subtype, ssid, control,
self.name = 'xray-vis'
self.level = 'L1'

@property
def dmin(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea this might be tricky as they are complex numbers

@samaloney samaloney mentioned this pull request Oct 7, 2024
@samaloney
Copy link
Collaborator

samaloney commented Oct 11, 2024

Ok normal test look good now but in the E2E tests at L1530 and L2008 don't seem related and are unexpected at least at first look.

@nicHoch nicHoch merged commit 4f693bb into i4Ds:master Oct 11, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants