-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add Zhu dust+molecule+atom table opacity variant and example file. #54
base: main
Are you sure you want to change the base?
Conversation
+ Add ZhuTableOpacity photon class with Rosseland-Planck databox. + Assume log-log rho-T for DataBox interpolation. + Add ZhuTable variant to opac_photons.hpp. + Add unit test for ZhuTable variant. Notes: - See: Zhaohuan Zhu et al (2021). - The next step is to convert the Zhu ASCII into HDF5 format(s).
@brryan @jonahm-LANL - can't seem to add reviewers - anyway here is a preliminary changeset - thanks for the help! |
+ Use filesystem header to check extention in ctor. + Refactor old ASCII parser to a private function (called if .txt). + Add ZhuTable::Save method and use to add HDF5 file. + Update unit test to parse HDF5 data instead. Thanks to Jonah Miller for the suggestion to use the Save method to create the HDF5 file.
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.
Looks good! Some minor suggestions but overall this seems like a pretty clean implementation. My main suggestion/complaint is to move the data files out of git. If we want to distribute them, we could maybe use github's release machinery. Let's discuss when you have the chance.
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.
not a huge fan of storing an ascii data file in the repo---but if this is the best solution we can work with it. It might be better to take a file name as input and distribute this file in a known place or have a script to fetch it. Same goes for the hdf5 binary. We could, if you like distribute them as artifacts in a data release via github's release mechanism. We do something similar for gold file data in singularity-eos.
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.
@Yurlungur - thanks very much for the comments - @brryan also recommended removing the files, and instead using a smaller "toy" test file - will make that change.
|
||
// construct Planck/Rosseland DataBox from Zhu ascii file | ||
ZhuTableOpacity(const std::string filename, const bool use_planck_absorb = false) | ||
: opac_type_(use_planck_absorb) { |
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.
Do you plan to load both types at once? It might be possible to generate them both with only one file load. Maybe like this:
ZhuTableOpacity(const ZhuTableOpacity &other, const bool use_planck_absorb = false)
: dist_(other.dist_), kappa_(other.kappa_), opac_type_(use_planck_absorb) {}
so you can initialize one variant from the other.
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.
Indeed, for use in transport specifically, was planning to permit even using both types at once in a hybrid-type scheme, which exists in the literature. This constructor suggestion seems like a nice way to get both variants for instance, for grey diffusion. Will add it...
# Link data file directory for Zhu table test | ||
file(CREATE_LINK | ||
${CMAKE_SOURCE_DIR}/singularity-opac/photons/zhu_data/opacitysolar09dustq3p5amax0p1new.txt | ||
${CMAKE_CURRENT_BINARY_DIR}/opacitysolar09dustq3p5amax0p1new.txt SYMBOLIC) | ||
file(CREATE_LINK | ||
${CMAKE_SOURCE_DIR}/singularity-opac/photons/zhu_data/opacitysolar09dustq3p5amax0p1new.hdf5 | ||
${CMAKE_CURRENT_BINARY_DIR}/opacitysolar09dustq3p5amax0p1new.hdf5 SYMBOLIC) |
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.
If we use a data release model, this would be replaced with cmake fetch content.
Co-authored-by: Jonah Miller <[email protected]>
Co-authored-by: Jonah Miller <[email protected]>
Notes: