-
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
Chunk size implementation for DIT Algorithm loops for Broadening #489
Conversation
Hello, good first work! |
Hello, sorry for getting back so late.
Then, I tested the basic example test_spectrum() as such:
The spectrum is calculated, however I do receive an error for the chunksize parameter:
Of course, this error doesn't occur if To test this example, clone my RADIS fork and run the test on your local machine. |
Update: the errors were being generated due to path errors in my Output:
The example is working for both |
Nice! Can you compare that you get the same spectrum with / without chunksize? Then can you try to push to large scale spectra (ex : CO2 HiTEMP, full range) and confirm that you can run it on limited RAM? |
Hello, I've confirmed that both the spectra are the same, with and without chunksize! P.S. Another thing, I'm having issues plotting through matplotlib due to a publib warning:
Due to which I'm not able to plot some spectra. But I'll try to get it resolved asap! |
Hello! You can change the default plotting style and library of Radis, drop publib and use something else. There is a Gallery Example showing how to customize plots |
It seems that this error is persisting even now. It's occurring on this line of _apply_lineshape_LDM(), giving a ValueError that all arrays should be of the same length. Also, here are the Pytest results after chunksize implementation: |
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.
See comments in code
also
- make sure you add in your PR objectives to also make the changes for nonequilibrium mode (AFAIK, there is a "broaden_lines_noneq" function somewhere) . Do this only after equilibrium works.
Here are the plot_diff() results for a HITEMP CO spectra, with the same parameters(optimization="simple"), the only difference being the chunksize. What I understand is happening:
|
Hello @sagarchotalia These results seems great ! Before moving to nonequilibrium we'll have to :
We also have to make sure it is more efficient. Computing all lines together is more efficient if all lines hold in RAM. It becomes problematic when you saturate RAM. With Chunksize, you have an implementation which will ensure you never saturate RAM.
After implementation you'll also be able to :
|
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.
See my Warning. The rest are minor comments
Making sure chunksize is taken into account in Spectrum Conditions, as per the suggestion by Erwan in radis#489 (comment) Co-authored-by: Erwan Pannier <erwan.pannier@gmail.com>
About the errors
|
Codecov Report
@@ Coverage Diff @@
## develop #489 +/- ##
===========================================
+ Coverage 73.10% 73.35% +0.24%
===========================================
Files 137 137
Lines 18870 18924 +54
===========================================
+ Hits 13795 13881 +86
+ Misses 5075 5043 -32 |
Looks good to me, merging this first major part of the GSOC project ! |
great @sagarchotalia 🎉 |
Congrats @sagarchotalia on the first major PR merge! |
Objectives
This pull request is to address the chunksize implementation in DIT Algorithm(optimized) loops in the _broaden_lines method.
With this PR, RADIS users shall be able to fully utilize the Chunksize feature in the code. Instead of looping the entire DataFrame, we use chunks of it at a time, hence allowing for much better memory management!
Changes Made
Future Objectives
Fixes #488