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

Different results for `run_until` and `run_until_and_store` #710

Closed
matthiasdusch opened this issue Mar 11, 2019 · 11 comments

Comments

Projects
None yet
3 participants
@matthiasdusch
Copy link
Member

commented Mar 11, 2019

Results for FluxBasedModel.run_until_and_store(y1) and FluxBasedModel.run_until(y1) can vary significantly.

More precisely the title should be "Different results if run_until is called with different time steps" but I think the most relevant appearance of this phenomena is when comparing results between run_until and run_until_and_store.

run_until_and_store internally calls run_until for every year of y1-y0. This is identical to
for y in range(y1-y0): run_until(y).
Therefor run_until always starts at the beginning of one year and stops at the end of it.

If run_until is called with a time span > 1yr it just uses the maximal possible time steps (as determined with a CFL condition) until y1 is reached. But these time steps can also begin in year y and end in y+1. And then the mass balance and/or elevation_feedback is off.

For many (probably most) glaciers this does not make a huge difference, but for others it can be significant . E.g this is Gepatschferner ('RGI60-11.00746'), run from from_prepro_level=4 with a temperature bias of -0.45, run for 320yr (on the x axis). y-Axis shows length change in meters.
run_until_differences

This problem can either be tackled in FluxBasedModel.step or probably better in FluxBasedModel.run_until. But should we? It would be more accurate but also slower for longer simulations while the effect on many glaciers would be marginal.

I did not want to touch those core functions without discussion first. Possible fixes:

  • Prevent run_until from 'crossing' years
  • Make it optional e.g by introducing a dt parameter
  • Make a fast and a slow function
  • Don't touch it, but document it (here and in the docs)
@fmaussion

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Thanks for the nice post!

I think it's reasonable to make run_until (or, maybe even better: the flowline model) stop at each MB year. On top of the reason you mention, another argument is the mass-balance: indeed, if we significantly exceed the first MB month with the last time step of the preceding year we are making a big difference.

So actually I have the feeling that the flowline model should have an option to force some steps, and the default should be to follow the updates of the mass-balance model.

This is again a very quick though from Reading class-room, but it sounds sensible to me.

cc @juliaeis who might be affected by this change as well.

@juliaeis

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Hey,
Thank you very much for the post! For my runs I use the run_until_and_store() function. A change in run_until() (as you suggested) would hence not affect my running times. In general, I think we should make sure that the two functions behave the same way. So an optional parameter in both functions would be the way I would prefer to. If I understand it correctly, this would then perhaps also speed-up my reconstruction method, as until know, I have used dt=1 ?!
If we would force the run_until function to stop at each MB year, why do we than have the CFL-condition? Would it not be better to also use this condition for the run_until_and_store() function or would this lead to an problem, because then our time steps, where we save some data would be irregular or to large?
No matter how we change this, an optional parameter would be really good! Because then, I could also use the old way, if I need.

@juliaeis

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

@matthiasdusch : Do you also have measured the times? So how large is the difference between the running times at your examples?

@fmaussion

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

In general, I think we should make sure that the two functions behave the same way.

Yes, this is why I actually suggest to implement it in step(), not any of the run_until* functions. The only drawback is that it will become harder to keep the other numerical implementations in line, but I don't think it's a real problem.

So an optional parameter in both functions would be the way I would prefer to. If I understand it correctly, this would then perhaps also speed-up my reconstruction method, as until know, I have used dt=1 ?!

No, it should be exact same. (and for the older default run_until, make it slightly slower)

If we would force the run_until function to stop at each MB year, why do we than have the CFL-condition? Would it not be better to also use this condition for the run_until_and_store() function or would this lead to an problem, because then our time steps, where we save some data would be irregular or to large?

I don't understand what you mean. But the CFL will only very rarely lead to steps larger than weeks or months, very often much shorter. We still need it for numerical stability, but the new check will just make sure that the step meets each new MB year's start.

No matter how we change this, an optional parameter would be really good! Because then, I could also use the old way, if I need.

if you used dt=1 before, this shouldn't change anything for you! (I hope ;-)

@matthiasdusch

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

run_until_and_store is just a wrapper which calls run_until year-by-year and stores the intermediate results. If we force the flowline model to "stop" after every MB year this will therefor not affect runs done with run_until_and_store.

The CFL condition is to determine the actual time step within the flowline model: Even if you run your model for 1 year (run_until(1)) this will be done in multiple time steps which are usually in the order of days.

As for the timing: Some very basic and non significant tests gave me around ~5% difference between run_until(yr) and for y in range(yr): run_until(y). So I think with a smart implementation in the flowline model the runtime differences will be way smaller than I initially thought.

run_until_and_store is a bit slower of course, as it also writes the data. If you are using this function (which you are and probably have to, as you are also interested in the intermediate results) these changes will probably not affect you. Neither positive nor negative :-)

Edit: This was meant as an answer to @juliaeis . But I got distracted while writing it so Fabi's answer came first.

@juliaeis

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Then, I'm fine with the changes.

@matthiasdusch

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

So I tried to tackle this issue and I came to a couple of conclusions. The first being: Gepatschferner, the glacier shown in the example above is just a really really weird glacier!
This plot shows the relative volume change for the same settings as above. I am really not sure what's happening there...
gep_vol1

Second conclusion: The difference might not be as big as I initially thought. The main reason being that OGGM's flowline model default timestepping sets an upper limit of 10 days. And even the 'ambitious' timestepping only uses 15 days as upper limit. So yes this can and does cause a difference, but it should be small for not so weird glaciers...

I implemented a limiting scheme (PR #743 ) and here are the length and volume differences between the current and my implementation for Hintereisferner :
len_diff
vol_diff
Length differences are within 1 grid point. Volume differences look weird again, but are in the order of 1e5 - 1e6 while the total volume is in the order of 1e9.

As HEF always works well, I randomly picked Oberer Grindelwald glacier and there the differences are basically none existing:
len_diff_ogr
vol_diff_ogr
Length difference is again one grid point, and volume difference is in the order of 10(!) m^3.

If we would proceed from here this for sure needs some more testing, but to be honest I am not even sure if it is necessary. Problems with glaciers like Gepatschferner will not be solved by this.

@fmaussion

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Thanks for the great write-up!

to be honest I am not even sure if it is necessary.

I would still like to proceed with the change as discussed. Regarding Gepatsch, it would be nice to find out what's going on indeed ;-)

@fmaussion

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@matthiasdusch wasn't this solved by #743 ?

@matthiasdusch

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Yes.

Gepatsch still does wired things. But at least there is no more difference between the timesteps now:
neu_len_gep
neu_vol_gep

@fmaussion

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Haha how weird is that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.