-
Notifications
You must be signed in to change notification settings - Fork 115
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
Fetch hitran update #505
Fetch hitran update #505
Conversation
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.
Many suggestions above.
Also tests are failing in all nonequilibrium calculations. You can test locally by computing a Spectrum with Tvib different from Trot
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.
minor comments
Codecov Report
@@ Coverage Diff @@
## develop #505 +/- ##
===========================================
+ Coverage 73.10% 73.30% +0.19%
===========================================
Files 137 138 +1
Lines 18870 19015 +145
===========================================
+ Hits 13795 13939 +144
- Misses 5075 5076 +1 |
@erwanp I was thinking of adding test cases to check if we are able to download the new parameters or not. But am not sure how exactly to implement them, because all the current tests that are being run must be downloading the original database, we can add a test by regenerating the database with extra_params = 'all' or how would you like to proceed with it? |
Talking about test cases, i suggested #505 (comment) Good idea to add the new test cases. We'll need them anyway. We shouldn't load a large database, because it will be downloaded on each CI run. But you can try with CO. Does CO have broadneing parameters by other species ? (CO2, etc. ? ) I'd add a |
…r parsing non-equilibrium parameters
Great work @Supriya1702, tested it locally, works fine on my end. The only thing missing will be adding a test to check if the |
…columns DataFileManager function in DataBaseManager
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.
LGTM
Well done ; I let you merge when the pipeline succeeds! |
This PR addresses-