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

adapt inversion flux and filter_inversion_output #1502

Merged
merged 30 commits into from
Jan 31, 2023

Conversation

pat-schmitt
Copy link
Member

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

Here I add an adapted version of filter_inversion_output.

The old implementation always smooths with a parabolic bed shape and overwrites the original cross-section shape. Further no smoothing was conducted if the 'smoothed-parabolic bed shape factor' was smaller than the minimum (cfg.PARAMS['mixed_min_shape']).

Now the adapted version preserves the cross-section shape. Additionally, if the 'smoothed-parabolic bed shape factor' is smaller than the defined minimum the cross-section shape is changed to a trapezoid (similar to what is done during the inversion with mixed bed shapes).

I opened this PR for discussion and to see which tests need to be adapted.

Further, the old implementation of this function probably causes the problem described in #1360, as the difference in the grid points is 5 grid points, which is the number of grid points which are smoothed and hence forced to a parabolic shape with the old function. And probably depending on the prepro-border the bed shapes of the downstream line are different (which partly decides if the inversion output is smoothed or not). But this needs further investigation.

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

@pat-schmitt
Copy link
Member Author

I have now undone my changes to filter_inversion_output and first, try to fix all tests by excluding the filter in the tests.

So far I managed to fix most of the test, but I would need help with two run_with_hydro tests, where I could not figure out the problem:

Further, some reference graphics need to be updated.

I also was wondering if I should also exclude the filter in all other tests which do not use init_hef. Many of these tests use workflow.inversion_tasks, which uses the filter by default. This probably would mean that many more reference graphics must be updated and probably some other tests adapted. The graphics must be updated anyway once I start changing the filter. But maybe it would be a good idea to exclude the filter completely from the reference graphics?

@pat-schmitt pat-schmitt mentioned this pull request Dec 2, 2022
4 tasks
@fmaussion
Copy link
Member

@pat-schmitt sorry for changing my mind over and over about this topic, but looking at the cross sections without filter I just think it still makes more sense to use it in the "default" workflow, until COMBINE becomes the default.

Would you be willing to continue this PR by adding the filter again in the HEF test case and try it on a few other glaciers with other bedshapes to see that it more or less works as expected?

I think that the new default should be to filter the inversion output and use dynamical spinup where appropriate

@fmaussion
Copy link
Member

For reference here is the section without filter:

image

And with filter:

image

Of course the filter was designed with Hintereisferner in mind, but maybe we can find something which "works" more or less, and by "working" i mean just not having crazy bed deepenings at the tongue...

@pat-schmitt
Copy link
Member Author

Here are five examples of the new filter I propose (not committed right now). The last example just shows that the filter is doing nothing if not necessary.

What do you think? Are there other glaciers I should test? Should I also include more test graphics or is HEF enough?

For Centerlines:

centerlines_filter

For Elevation bands:

elev_bands_filter

@fmaussion
Copy link
Member

This looks great! Let's go for it.

@pat-schmitt
Copy link
Member Author

@fmaussion we discussed that I should try to delete the artificial increase of the ice flux at the last grid point (https://github.com/OGGM/oggm/blob/master/oggm/core/inversion.py#L116), now I was wondering if I also should delete the same thing for the tributaries just below (https://github.com/OGGM/oggm/blob/master/oggm/core/inversion.py#L120) or only for the main flowline.

@fmaussion
Copy link
Member

@pat-schmitt yeah delete there also - better not inventing stuff

@pat-schmitt
Copy link
Member Author

Without the artificial flux increase, you see a small hill at the last grid point (because of zero ice thickness through construction). (If we keep this we should adpte the check for zero thickness from <= to < https://github.com/OGGM/oggm/blob/master/oggm/core/inversion.py#L532)

I adapted my filter now and these are the results: (Should I continue adapting the tests again?)

Centerlines:
centerlines_filter

Elevationbands:
elev_bands_filter

@fmaussion
Copy link
Member

@pat-schmitt I trust your judgment here. Either put back the flux at 1/10, or maybe (probably better) find a way to make it look better using your idea of prolongating the immediate downstream slope?

As a general rule I think the prolongation of the downstream slope to avoid deepenings or bumps is very clever.

@pat-schmitt pat-schmitt changed the title adapt filter_inversion_output to conserve the cross section shapes adapt inversion flux and filter_inversion_output Jan 24, 2023
@pat-schmitt
Copy link
Member Author

I tested my new idea of the implementation of the inversion flux now (see two examples below). If the terminus is very thin (first row) there is still a small artefact present. However, it looks better at a 'thicker' terminus (second row, right subplots), without the need for an artificial flux.

With this, I will now start fixing the tests (the idealized tests passed right away).

compare_inversions

@fmaussion
Copy link
Member

👍 perfect - any artifact at the tongue will have to be addressed with filtering anyway. Thanks!

@pat-schmitt
Copy link
Member Author

I managed to fix all tests besides one (and the graphics).

Something with the correction of negative fluxes is not working out in the mu_star_calibration (when an individual mu* is calibrated for 'negative-flux flowlines'). In the code (https://github.com/OGGM/oggm/blob/master/oggm/core/centerlines.py#L286), it says 'we can cope with one negative flux grid point', but I do not understand how this is handled. I understand the recursive calibration (https://github.com/OGGM/oggm/blob/master/oggm/core/climate.py#L1121) starting from the top with fixing the individual mus*, but why is one negative flux grid point ok? Has this one negative flux grid point zero thickness after the inversion?

Besides this: for calving flowlines, I decided to use the old implementation so that the thickness of the last grid point is consistent with the calving parametrization.

@fmaussion
Copy link
Member

but why is one negative flux grid point ok? Has this one negative flux grid point zero thickness after the inversion?

I think it was arbitrary - sometimes the last grid point was negative for numerical reasons, and it will be clipped to zero and I found this not too bad. I think you can take another decision here.

Besides this: for calving flowlines, I decided to use the old implementation so that the thickness of the last grid point is consistent with the calving parametrization.

OK

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.

I skimmed through the code - this looks good I think?

@fmaussion fmaussion merged commit bf072b2 into OGGM:master Jan 31, 2023
@pat-schmitt pat-schmitt deleted the adapt_filter_inversion_output branch January 31, 2023 15:32
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.

None yet

2 participants