search

cholla-hydro / cholla

A GPU-based hydro code
https://github.com/cholla-hydro/cholla/wiki
MIT License
66 stars 32 forks source link

Dust Build #209

Closed helenarichie closed 1 year ago

helenarichie commented 1 year ago

Contains code for dust model (sputtering only) that can be turned on using the DUST build type and initial condition/boundary condition functions (Grid3D::Clouds() and Grid3D::Wind_Boundary()) for cloud in wind tunnel set up

evaneschneider commented 1 year ago

This mostly looks good to me, with a couple recommended changes: -For features that are not "generic", but rather specific to a problem, I think we should leave them as the defaults, instead of adding more macros (so for example, leave the default temperature floor and min_dt_slow, so that it's consistent with the documentation, and instead change it in a particular problem if you need to). Similarly, it's not clear to me that we need to change the cloud ICs, but I do think it's useful to have in initial conditions example of how to set the dust density -For the dust module itself, equations that come from papers should be commented to reflect their origin (I'm thinking for example of the tau_sp equation)

evaneschneider commented 1 year ago

Oh, and can you please confirm that all existing tests all still pass with this code addition?

bcaddy commented 1 year ago

Oh, and can you please confirm that all existing tests all still pass with this code addition?

This might be tricky. PR #199 unintentionally removed the system test data submodule. Until that is back system tests don't have data to compare to. That was @ojwg's PR and I have alerted him to the issue.

evaneschneider commented 1 year ago

PR #199 unintentionally removed the system test data submodule. Until that is back system tests don't have data to compare to. That was @ojwg's PR and I have alerted him to the issue.

Oh dear. All the more reason to have more of us doing PR reviews!

helenarichie commented 1 year ago

ca4f7d7 removed the dust build-specific modifications to TEMP_FLOOR and min_dt_slow, and 11f2477 added citations to the dust code and removed modifications to the Clouds() initial conditions function (except for setting the dust density if #DUST is defined). I'm checking the tests now and will leave another comment when that's done.

helenarichie commented 1 year ago

Okay, I manually downloaded and copied the system tests data from #198 (the last PR before it was deleted) into my repo, and verified that the tests pass for the hydro and dust builds.