-
Notifications
You must be signed in to change notification settings - Fork 127
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
Leftover chemistry fixes #553
Comments
5 tasks
Thank you Kyle and Kengo!!! That's a great new year gift from you :)
I took notes of the issues and will fix them a bit later since they seem to
be minor.
…On Fri, Dec 29, 2023 at 4:08 AM Kyle Gerard Felker ***@***.***> wrote:
I merged #496 <#496>
since it was 99% complete thanks to @munan <https://github.com/munan>,
happy new years everyone! GitHub's servers also seemed to be struggling
with the long PR history and review comments, which were impossible to
decipher given all the interleaving changes. I will document a few of the
unaddressed comments here, which we can address now or eventually:
- @tomidakn <https://github.com/tomidakn>
- inputs/chemistry/athinput.chem_tigress might be better excluded
from the source code, since it requires TIGRESS data?
- src/utils/interp.cpp function names like LP1D, LP2Di, etc. are
not self explanatory. Use longer names. (also, the Doxygen comment says "
ID arrray linear interpolation", with typos at 1D and "array")
- src/units/units.cpp, regarding things like code_time_cgs_ defined
in this folder, "It is better to minimize problem specific codes in the
master branch. Technically this can be done in the input file for each
problem (i.e., always "custom"). Is there any reason to have them hard
coded?"
- @felker <https://github.com/felker>
- inputs/chemistry/athinput.pdr_moving and other input files have
<problem> block input variables like Tw which are seemingly unused
in the problem generator file. If there is a reason for still keeping them
in the input file, it should be documented in a comment
—
Reply to this email directly, view it on GitHub
<#553>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVXJH2C64SHTWCWFGMPGNLYLXGUVAVCNFSM6AAAAABBF3NEDOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGA2TQNZYGQ4DENY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Merged
delaying until after the 24.0 release |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I merged #496 since it was 99% complete thanks to @munan, happy new years everyone! GitHub's servers also seemed to be struggling with the long PR history and review comments, which were impossible to decipher given all the interleaving changes. I will document a few of the unaddressed comments here, which we can address now or eventually:
inputs/chemistry/athinput.chem_tigress
might be better excluded from the source code, since it requires TIGRESS data?src/utils/interp.cpp
function names likeLP1D, LP2Di
, etc. are not self explanatory. Use longer names. (also, the Doxygen comment says " ID arrray linear interpolation", with typos at 1D and "array")src/units/units.cpp
, regarding things likecode_time_cgs_
defined in this folder, "It is better to minimize problem specific codes in the master branch. Technically this can be done in the input file for each problem (i.e., always "custom"). Is there any reason to have them hard coded?"inputs/chemistry/athinput.pdr_moving
and other input files have<problem>
block input variables likeTw
which are seemingly unused in the problem generator file. If there is a reason for still keeping them in the input file, it should be documented in a commentThe text was updated successfully, but these errors were encountered: