-
Notifications
You must be signed in to change notification settings - Fork 47
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
Alternate Stage 3 Inputs #603
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #603 +/- ##
==========================================
- Coverage 54.29% 54.28% -0.01%
==========================================
Files 101 101
Lines 12713 12779 +66
==========================================
+ Hits 6902 6937 +35
- Misses 5811 5842 +31 ☔ View full report in Codecov by Sentry. |
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.
Great job with the tutorial! Most of the requested changes are pretty small, but we should talk about placing default values into the MetaClass.
docs/source/tutorials/formatting_eureka_s4_compatible_data.ipynb
Outdated
Show resolved
Hide resolved
docs/source/tutorials/formatting_eureka_s4_compatible_data.ipynb
Outdated
Show resolved
Hide resolved
docs/source/tutorials/formatting_eureka_s4_compatible_data.ipynb
Outdated
Show resolved
Hide resolved
@kevin218 , @taylorbell57 Okay I made some tweaks based on the comments. Sorry if I couldn't get absolutely everything done, I think we at least have a good foundation to build from for more specific custom data inputs and / or changes to the tutorial. I don't think I'll have a chance to put more work into this, so if there's any more tweaks you'd like to make, it would probably be best to checkout my branch and incorporate them directly - I'll be sure to leave it as it currently is! 😄 |
@taylorbell57 Do you also want to review changes to this PR before merging into main? |
Yes please, I'll likely do so this weekend or on Monday |
I know you're busy right now @AarynnCarter, so I'll try to address my own comments in the next week or so |
Thanks @taylorbell57, sorry to drop the PR on you not quite finished! |
This should let users continue to use old Meta_Save.dat files but supports the newer storage method of the SpecData.h5 file
S3 SpecData files without a mask will now mask any NaN values
Alright, I've fixed most of the known issues with the PR. I still need to review the new tutorial and potentially make some changes to the S4 checks (depending on whether this PR gets merged after or before PR #632). |
@AarynnCarter, I fixed some very minor typos in your excellent tutorial, but there was also an error in how you setup the photometry SpecData.h5 file. To be sure that Stage 4 functions properly, the I believe my implemented change should work well, but I need you to re-run the notebook to update all the notebook outputs; hopefully this should take only ~1 minute of your time. It's possible that I should've done |
@taylorbell57 No worries, I'll aim to update things by the end of next week! |
@taylorbell57 Okay, all done! You were right about it needing to be cast as a numpy array. There was also a typo I fixed in the allocation of the x value. |
Pull request to allow for non-Eureka! inputs to Stage 4.
Leaving for a future PR:
This PR resolves part of #404