-
Notifications
You must be signed in to change notification settings - Fork 192
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
Import FUKA initial data #5339
Import FUKA initial data #5339
Conversation
@ermost want to give this a try? When loading some sample ID in the FUKA repo I'm getting an FPE in Kadath, but that might be because my domain wasn't set up correctly or something like that. |
get<0, 1>(spatial_metric) = to_datavector(fuka_data[5]); | ||
get<0, 2>(spatial_metric) = to_datavector(fuka_data[6]); | ||
get<1, 1>(spatial_metric) = to_datavector(fuka_data[7]); | ||
get<0, 2>(spatial_metric) = to_datavector(fuka_data[8]); |
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.
should this be 1,2?
It seems like you are not setting gamma_yz?
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.
Thanks, fixed! The FPE happens in Kadath though, so I don't think this was it..
Also I'd like to add a test for this that is enabled only when FUKA is available. Maybe we could put some sample data into the FUKA install directory? Do you think some of the data in the FUKA repo would be appropriate, or should we generate new data?
Sure, happy to take a look. |
56936b9
to
c5cb1e1
Compare
b3ea2d1
to
b0fe0a0
Compare
This is ready for review now. |
03bb691
to
b11ce16
Compare
Can't fix the remaining clang-tidy warnings I think. |
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, please squash
static constexpr Options::String help = { | ||
"Path to the '.info' file of data produced by FUKA."}; | ||
}; | ||
|
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.
Should we add a comment here or in the help string that the constant electron fraction option is most of the time inconsistent with the ID, and will need to be replaced later?
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.
Yes, let's do that! Does the ID write out consistent a electron fraction? If not, should that be added? We've talked about this for SpEC too, and that's the plan for spec
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.
Neither electron fraction nor temperature are written out by FUKA. I added a note to the dox for this class and opened #5518.
Co-authored-by: Samuel Tootle <[email protected]>
Proposed changes
On CaltechHPC load the
fuka/2023-08
module (inmodule use /central/groups/sxs/modules
), then configure the CMake build (or setFUKA_ROOT
yourself).To use this set
InitialData
in a GHMHD input file to something like this:Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments