-
Notifications
You must be signed in to change notification settings - Fork 3
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
Refactor parsing of results files #176
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #176 +/- ##
==========================================
+ Coverage 70.41% 71.50% +1.08%
==========================================
Files 11 13 +2
Lines 730 800 +70
==========================================
+ Hits 514 572 +58
- Misses 216 228 +12 ☔ View full report in Codecov by Sentry. |
91f7f3c
to
ba82ec1
Compare
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.
A few minor suggestions that you can ignore if you want. This looks otherwise fine to me! Thanks for putting this together.
name='average ion mass for a H-He plasma', | ||
value=2.171e-24, | ||
unit='g', | ||
uncertainty=0, |
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.
There's definitely an uncertainty, but I don't know what it is!
The uncertainty from the abundance set (likely Feldman) + the uncertainty of the hydrogen and helium masses should propagate.
Perhaps we should make an issue to figure this out? I do like that this has the option of including it!
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.
Good point. I had this thought when I put in 0 for the uncertainty but didn't have any idea as to what this value may be. We can create another issue to track this.
Fixes #86
This pull requests does a few things though is mainly focused on refactoring the parsing of the results files to try and simplify things:
pydrad/parse/util.py
that returnastropy.table.QTable
objects rather than arraysStrand
are only dynamically generated if there are many of them or if they are unknown ahead of time (e.g. for the.ine
files). Otherwise they are explicitly listed onProfile
..amr
files. If the.phy
files are not read (or not present), the thermodynamic quantities are computed from the conserved quantities in the.amr
files. See Function for calculating thermodynamic variables from conserved variables #86.TODOs:
Profile
.amr
file has an additional column for the electron mass density