Add capability to reset the "state" of the mass balance model

[dngoldberg]

Although this does not apply to any existing mass balance models, there may be mass balance models in the future that have a "state" associated. Now, for a given set of climate files for a given glacier in a given year or interval, the mass balance models return consistent values whenever get_annual_mb or get_specific_mb is called. But if an energy balance model is implemented, the mass balance will depend on properties such as snow state and glacier surface temperature, and this may not give consistent results unless there is a consistent "state". The issue arises, for instance, in apparent_mb_from_any_mb, but there could be others as well.

One possibility is to save the "state" in a given year in a glacier directory. However, this will not completely address the problem as get_annual_mb may be called for a different year.

Another is to give flexibility by allowing a "reset_state" kwarg. This does absolutely nothing with existing mass balance models, but allows developers of new MB models a way to address the issues above. That is what the current pull request does.

NOTE a far less-invasive option to address the issue in apparent_mb_from_any_mb might be to re-instantiate the mass balance model within the function. But this led to multiple testing errors.

Closes #1755

• Passes All ci Tests
• Fully documented
• Entry in whats-new.rst: Added "reset_state" functionality to MassBalanceModel.get_specific_mb() to
  signal any state-dependent mass balance model to "reset its state" at the start
  of the period. This is achieved through a "reset_state" kwarg passed by
  MassBalanceModel.get_annual_mb(). Done to address that
  apparent_mb_from_any_mb() calls get_specific_mb() twice, leading to nonzero
  terminal flux (potentially) with a state-dependent mass balance model.
  apparent_mb_from_any_mb() also changed accordingly. Similar change also
  made to MultipleFlowlineMassBalanceModel.get_specific_mb(). Does not impact
  any current functionality within oggm.

NOTE: this PR is invasive, and i have decided on a less invasive change. so i am closing it.

[pep8speaks]

Hello @dngoldberg! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2025-01-17 10:51:02 UTC

[fmaussion]

@dngoldberg thanks for this. Here is an alternative suggestion which I think is a bit cleaner. Changes:

• reset_state() is now a method of any MB model. It does nothing per default.
• reset_state() is called each year if asked for. I still think it's a bit wild to do that, but I can see how this can be useful, if anything for testing at least

The current way that apparent_mb_from_any_mb computes the mass-balance twice is still not optimal. We really don't need to do that, and I'll look into this in a separate PR (should not be too much work).

Let me know if this works for you as suggested by my last commit.

[dngoldberg]

hi @fmaussion thanks very much and i will look at your change soon. But i want to say, i was thinking about it further and maybe the issue that led to this PR could be addressed simply at the level of the "state"-ful MB model through implementing get_specific_mb() instead of using the parent class.

at the same time, though, you mentioned that a MB model should always be reproducible and the output of get_annual_mb() should not depend on previous calls. this PR is meant to address this which is why i decided to submit it after all. But if you think this is less important, i could close without merging. Please let me know.

i need to understand apparent_mb_from_any_mb() better, my suspicion is that it does NOT need to call get_specific_mb() but i will look carefully. (that would, also, address the issue that precipitated this PR -- but, of course, there might be other issues not yet encountered, such as in dynamic spinup..)

[fmaussion]

@dngoldberg the current PR is totally fine by me. It should do exactly what you wanted it to do (resetting a state each year when calling multiple years), but a bit differently.

my suspicion is that it does NOT need to call get_specific_mb()

I agree with that ;-) - I've just been lazy. The second call to MB on the flowlines is what's needed. It can be avoided.

[dngoldberg]

hi @fmaussion have looked it over. while this does solve the apparent_mb_from_any_mb() issue, it is not quite what i wanted, because in the specific implementation that I am thinking of the state is important when modelling mb over a multi-year period. this is why i was only resetting the state at the beginning of get_specific_mb().

However -- i like your addition of a reset_state() function in the parent class. Therefore I propose this:

1. we keep the reset_state() that does nothing in the parent class
2. undo any changes to get_specific_mb()
3. Just for now, we add a reset_state() call in the middle of apparent_mb_from_any_mb(). this will make the two calculations of mean mass balance over the apparent_mb period equivalent, which was the main problem i was concerned with. The changes are far less intrusive
4. create an issue to refactor apparent_mb_from_any_mb()
5. the reset_state() method can then be used for any state-dependent mb model whenever it is needed.

I don't know a clever way to (1) and (2) without just undoing the commits and adding the reset_state() function manually. but if you agree with the above, and you know a quick way, feel free!

[dngoldberg]

hi @fmaussion what do you think of these changes? i like it because it's far less extensive. all tests pass, and it addresses the issue i was having with apparent mass balance. refactoring apparent_mb_from_any_mb() might be beyond the scope of this PR, but an issue could be created. meanwhile, this gives more flexibility.