-
Notifications
You must be signed in to change notification settings - Fork 8
Add divdamp order options except 24 (add 2 and 4) #1046
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
base: main
Are you sure you want to change the base?
Conversation
|
cscs-ci run default |
1 similar comment
|
cscs-ci run default |
vectorflux
left a comment
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.
Seems good to me as long at the OR condition in solve_nonhydro.py is definitely correct.
| if self.iadv_rhotheta != dycore_states.RhoThetaAdvectionType.MIURA: | ||
| raise NotImplementedError("iadv_rhotheta can only be 2 (Miura scheme)") | ||
|
|
||
| if self.divdamp_order != dycore_states.DivergenceDampingOrder.COMBINED: |
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.
So if I understand correctly, the 24 limitation did not really exist, in spite of this error message.
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.
Well, at least 2 was not correctly implemented.
4 might already have worked, yes.
| and second_order_divdamp_scaling_coeff > 1.0e-6 | ||
| self._config.divdamp_order == dycore_states.DivergenceDampingOrder.SECOND_ORDER | ||
| or ( | ||
| self._config.divdamp_order == dycore_states.DivergenceDampingOrder.COMBINED |
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.
I'm not sure I understand how this ensures 2 and 4 are also allowed, but I'll take your word for it.
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.
This just adds the logic for 2 or SECOND_ORDER exactly how it is in the ICON Fortran code.
This logic path in the logic tree was missing ...
|
cscs-ci run default |
|
cscs-ci run default |
|
Mandatory Tests Please make sure you run these tests via comment before you merge!
Optional Tests To run benchmarks you can use:
To run tests and benchmarks with the DaCe backend you can use:
To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:
For more detailed information please look at CI in the EXCLAIM universe. |
|
cscs-ci run distributed |
No description provided.