athena icon indicating copy to clipboard operation
athena copied to clipboard

Leftover chemistry fixes

Open felker opened this issue 2 years ago • 2 comments

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:

  • @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
    • 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

felker avatar Dec 28 '23 20:12 felker

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 https://github.com/PrincetonUniversity/athena/pull/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 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 https://github.com/PrincetonUniversity/athena/issues/553, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVXJH2C64SHTWCWFGMPGNLYLXGUVAVCNFSM6AAAAABBF3NEDOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGA2TQNZYGQ4DENY . You are receiving this because you were mentioned.Message ID: @.***>

munan avatar Jan 15 '24 02:01 munan

delaying until after the 24.0 release

felker avatar Apr 30 '24 16:04 felker