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

adding semi-implicit scheme #1507

Merged
merged 10 commits into from
Dec 15, 2022

Conversation

pat-schmitt
Copy link
Member

@pat-schmitt pat-schmitt commented Nov 22, 2022

In this PR I added the semi-implicit numeric scheme developed by @dngoldberg.

I also added the scheme to the idealised test cases. Further, I compared the FluxBasedModel and the SemiImplicitModel on a mixed flowline with different lambdas along the flowline (including a mixture of rectangular and trapezoidal).

Some comments and things to discuss:

  • in test_min_slope you can nicely see the instability differences in the slope plot
  • in test_cliff the volume difference to the MUSCLSuperBeeModel is only the half for the SemiImplicitModel compared to the FluxBasedModel, but it looks like their are some instabilities you can see in the surface_h plot for the SemiImplicitModel, maybe something to have a closer look
  • I have not implemented a MassConservationChecker (as I check all the time against the FluxBasedModel, which is checked), would this be a good idea?
  • I have not included a test for the sliding parameter
  • I have not included tests on real glacier geometry (could also include sliding tests there), what would be the most important real geometry tests?
  • the used cfl number right now is 0.6 (I came up with this by testing on an unstable glacier for COMBINE), but maybe also would need more testing/justification
  • currently, some diagnostics are calculated each timestep, which are actually not needed. They could be calculated somewhere else for increasing the performance

ToDo:

  • idealised Tests added/passed
  • real geometry Tests added/passed
  • Fully documented
  • Entry in whats-new.rst

@pat-schmitt pat-schmitt changed the title adding semi-implicit scheme with idealised test cases adding semi-implicit scheme Nov 22, 2022
@pat-schmitt
Copy link
Member Author

pat-schmitt commented Nov 24, 2022

Now the diagnostics are calculated using @property decorators to avoid unnecessary calculations during the dynamic run. This is needed because some diagnostics are not computed during the dynamic run (as is the case for FluxBasedModel).

@pat-schmitt
Copy link
Member Author

I now added tests using HEF.

For this, I adapted the test glacier directory (init_hef()) to be able to select which flowline type you want to use for the test. If you want to use the elevation bands in your tests you can now use hef_elev_gdir.

Probably some of my included tests need to be adapted when filter_inversion_output is not used anymore (see #1502)

Copy link
Member

@fmaussion fmaussion left a comment

Choose a reason for hiding this comment

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

Fantastic! Please just check camelcase parts of code and this is good to go - can't wait to use it!!!!

@pat-schmitt pat-schmitt merged commit d0d48f6 into OGGM:master Dec 15, 2022
@pat-schmitt pat-schmitt deleted the add_semi_implicit_scheme branch January 17, 2023 09:18
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.

2 participants