Skip to content
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

Corrector stencil 60 #350

Merged
merged 11 commits into from
Jan 19, 2024
Merged

Corrector stencil 60 #350

merged 11 commits into from
Jan 19, 2024

Conversation

halungge
Copy link
Contributor

(Fix): Add missing call to mo_solve_nonhydro_stencil_60 in corrector step.

Fix control flow related to mo_solve_nonhydro_stencil_59 (predictor) and mo_solve_nonhydro_stencil_60 (corrector): driver code determines whether first or last substep is run, since it controls the entire substep loop.

@halungge
Copy link
Contributor Author

cscs-ci run default

@halungge
Copy link
Contributor Author

launch jenkins spack

@OngChia
Copy link
Contributor

OngChia commented Jan 16, 2024

I think it is nice to have the at_first_substep and at_last_substep swithces.
Similar to config parameter ndyn_substeps_var in solve_nonhydro, I noticed that there is also a config parameter ndyn_substeps in diffusion. Because the number of substeps may dynamically change during runtime, I wonder that whether those variables in initialization of diffusion that use ndyn_substeps are also affected. If so, perhaps it can also be added as a TODO there in the future.

@halungge
Copy link
Contributor Author

I think it is nice to have the at_first_substep and at_last_substep swithces. Similar to config parameter ndyn_substeps_var in solve_nonhydro, I noticed that there is also a config parameter ndyn_substeps in diffusion. Because the number of substeps may dynamically change during runtime, I wonder that whether those variables in initialization of diffusion that use ndyn_substeps are also affected. If so, perhaps it can also be added as a TODO there in the future.

I agree. I did not know at the time that it could change dynamically. We should have a do a consolidation "pass through" with just this ndyn_substeps business in mind and fix it consistently everywhere.

@halungge
Copy link
Contributor Author

launch jenkins spack

@halungge
Copy link
Contributor Author

cscs-ci run default

Copy link
Contributor

@abishekg7 abishekg7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

def _is_last_substep(self, step_nr: int):
return step_nr == (self.n_substeps_var - 1)

@staticmethod
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this staticmethod and not the other one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other accesses the class content, so you need the `self

@halungge
Copy link
Contributor Author

launch jenkins spack

Copy link

Mandatory Tests

Please make sure you run these tests via comment before you merge!

  • cscs-ci run default
  • launch jenkins spack

Optional Tests

To run benchmarks you can use:

  • cscs-ci run benchmark

In case your change might affect downstream icon-exclaim, please consider running

  • launch jenkins icon

For more detailed information please look at CI in the EXCLAIM universe.

@halungge
Copy link
Contributor Author

cscs-ci run default

@halungge halungge merged commit 9740f6f into main Jan 19, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants