NCAR / icar

The Intermediate Complexity Atmospheric Research model (ICAR)
MIT License
72 stars 53 forks source link

Bertjebertjek's PR-126 #128

Closed scrasmussen closed 2 years ago

scrasmussen commented 2 years ago

This is PR-126! Some weird git stuff happened...

TYPE: bug fix, enhancement,

KEYWORDS: SLEVE, gamma factor, batch submit scripts, ideal test case

SOURCE: Bert Kruyt, NCAR

DESCRIPTION OF CHANGES: The SLEVE gamma factor was calculated incorrectly (did not account for n!=1). This did not have any effect on model functioning, only reported value at runtime was incorrect. Now the correct value is displayed on domain initialization.

Also, a bit more feedback in case of negative gamma is given.

Batch submit scripts: Changes suggested by Ethan w.r.t. previous PR have been incorporated.

gen_ideal.py & associated files: first implementation of more complex structures for ideal testcase. Now has the ability to generate more than one hill, and I've added more flexibility w.r.t. namelist options and topography. This will eventualy include an advection test modeled after the SLEVE paper by C.Schär et al 2002. But this requires some modification of the advection code (basically allowing the model to have QV without the advection code running). To be continued. The current implementation works fine though.

ISSUE: Partially adresses comments to PR 124 (that I've closed, now you can just merge this one).

TESTS CONDUCTED: Runs as before, no changes to dynamics, only diagnostic (apart from the ideal case that is).

Checklist

gutmann commented 2 years ago

@scrasmussen I don't think @bertjebertjek can make changes to this anymore since it is "your" PR at the moment. Are there any remaining items that haven't been fixed?

scrasmussen commented 2 years ago

Just wanting to note that we might want to rename tests/gen_ideal_test.py to tests/gen_sleve_ideal_test.py or something similar. Then we can leave tests/gen_ideal_test.py as a base test and individuals can use it to copy and create more complex test cases.

I think the best way to do this so we keep the commit history is to move the current gen_ideal_test.py to gen_sleve_ideal_test.py with git mv. Then wget https://raw.githubusercontent.com/NCAR/icar/main/tests/gen_ideal_test.py and add and commit gen_ideal_test.py. The history of gen_ideal_test.py will be lost but the sleve test git history will be preserved. There are ways to preserve both but they can get complicated and I don't think there is a need.

gutmann commented 2 years ago

Good point @scrasmussen would you open an issue to that effect so we can track that idea without slowing the merge of this PR? I'd like to see the gen_ideal_test be very easy to modify what test you want. This could be CLI arguments, or a parameter file, or a few different ideal_test cases (but I think there are a HUGE potential number of them).

scrasmussen commented 2 years ago

@scrasmussen I don't think @bertjebertjek can make changes to this anymore since it is "your" PR at the moment. Are there any remaining items that haven't been fixed?

When I made it the PR I actually gave @bertjebertjek permissions so he can edit by pushing to the branch on my fork. If that isn't working let me know Bert!

Good point @scrasmussen would you open an issue to that effect so we can track that idea without slowing the merge of this PR? I'd like to see the gen_ideal_test be very easy to modify what test you want. This could be CLI arguments, or a parameter file, or a few different ideal_test cases (but I think there are a HUGE potential number of them).

I'll open an issue so its noted but won't slow down the merge 👍
And I'll also start thinking about simple ways to generated ideal test cases