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

WIP: Allow different mu's for different flowlines #539

Merged
merged 14 commits into from Oct 20, 2018

Conversation

Projects
None yet
3 participants
@fmaussion
Copy link
Member

commented Sep 29, 2018

  • Fixes #478
  • Tests added/passed
  • Fully documented, including whats-new.rst for all changes

This is a longstandig issue and not an easy one.

I think I've managed to find an elegant way to deal with the calibration: it uses recursive calls to the calibration to calibrate the flowline's mu* until the entire glacier is done.

This still needs more test, and the mass-balance models need an update, hence still a WIP

@pep8speaks

This comment has been minimized.

Copy link

commented Sep 29, 2018

Hello @fmaussion! Thanks for updating the PR.

Comment last updated on October 20, 2018 at 13:23 Hours UTC
@matthiasdusch

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

The recursive approach is indeed elegant.

I have one comment/question regarding readability and also performance:
With this commit, only negative-flux-flowlines will get a different mu_star. So if all tributary flowlines are good, it should be possible to immediately use the already calculated mu_star_glacierwide. The _mu_star_per_minimization would then only be called on recursive calls if one or more flowlines need corrections.

But I might miss a point here or you have already some future adaptions in mind.

@fmaussion

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2018

Thanks for looking into it!

Yes, there are some issues regarding performance here. Even the call to "minimize" is superfluous altogether since the problem is linear in mu anyway.

However, I would like to avoid doing premature optimizations, following the good old adage*. They add complexity (here, at least one "if" and probably more), and we don't know yet if it's so slow.

*: "The First Rule of Program Optimization: Don’t do it.
The Second Rule of Program Optimization (for experts only!): Don’t do it yet"

@fmaussion fmaussion force-pushed the fmaussion:oggm-dev branch from 2129c88 to 53cf2be Oct 10, 2018

@fmaussion

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

This is slowly taking shape - I have added a new class to the mass-balance models which handles multiple flowlines. I spent the entire day on it - not because it's a lot of code (it's not that much code at all :D) but because it was quite hard to find the right API. I hope this is the right one now...

Anyways, still need to make this new MB the default in the production runs and also need to get the cross-validation working.

conftest.py Outdated
@@ -1,6 +1,3 @@
import matplotlib
matplotlib.use('Agg') # noqa: E402

This comment has been minimized.

Copy link
@matthiasdusch

matthiasdusch Oct 11, 2018

Member

shouldn't this be part of PR #542 ?

This comment has been minimized.

Copy link
@fmaussion

fmaussion Oct 11, 2018

Author Member

ups. Yes.

year=None):

if heights is not None or widths is not None:
raise ValueError('`heights` and `heights` kwargs do not work with '

This comment has been minimized.

Copy link
@matthiasdusch

matthiasdusch Oct 12, 2018

Member

heights and widths kwargs...

This comment has been minimized.

Copy link
@fmaussion

fmaussion Oct 12, 2018

Author Member

thanks!

@fmaussion

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2018

OK, the last commit is bigger than I wish it would, BUT:

  • I now added an option to compute the reference t* using the multiple flowlines approach. It is much slower than using the glacier wide one for only minimal differences in the bias (not even in the t*)! So this was done for nothing, other than helping my conscience ;-). The default remains to use glacier-wide t* calibration.
  • The climate workflow has changed it's name, I'd be happy to hear about your thoughts on this
  • compute_ref_tstar is now done in parallel

So, why changing the API again? Let me try to explain.

Before, mu_candidates and t_star_from_refmb were separate tasks. It didn't make much sense, because they always needed to be called together: t_star_from_refmb needs mu_candidates and mu_candidates without t_star_from_refmb is useless.

Now, the mu candidates are computed and used in the same task (which kept its name): t_star_from_refmb . The mu_candidates task is still available for test and documentation purposes, but it is not used in a normal workflow. To make sure there is no confusion, I renamed it to glacier_mu_candidates (could be another name though).

Since I was changing things already, I reconsidered the names of the other climate calibration tasks:

  • local_mustar is actually interpolating t* from the reference table. For convenience, it is also calculating the glacier wide mu*
  • apparent_mb was using the local mu* to add the mass flux to the branches for the inversion. Now, it is actually calculating the multiple flowline's mu*

In my last commit, I renamed local_mustar to local_tstar and apparent_mb to mu_star_calibration to better reflect what they do.

Now that I think about it, I believe they could even be merged in one big single task, which does both the interpolation of t* and the mu* calibration in one step. I'll consider this option, more on this later...

@fmaussion

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

@TimoRoth for some reason the graphics tests pass with 3.7 but not with 3.6 -> this is suspicious .

Any idea what's going on?

@OGGM OGGM deleted a comment from coveralls Oct 19, 2018

@OGGM OGGM deleted a comment from coveralls Oct 19, 2018

@OGGM OGGM deleted a comment from coveralls Oct 19, 2018

@OGGM OGGM deleted a comment from coveralls Oct 19, 2018

@OGGM OGGM deleted a comment from coveralls Oct 19, 2018

@fmaussion

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

OK, Travis is green, let's merge this - sorry @OGGM/oggm-devs for all the changes. I suggest not to update until V1.1 is out (next couple of weeks), after which we will stabilize the code base (I promise ;)

@fmaussion fmaussion merged commit 0a25564 into OGGM:master Oct 20, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sumnonpuella added a commit to sumnonpuella/oggm that referenced this pull request Oct 21, 2018

WIP: Allow different mu's for different flowlines (OGGM#539)
* WIP: allow multiple mu* per glacier

* Typo

* Add test for multiple branches

* First API that looks OK

* Revise the get_specific_mb signature before rename

* Rename and make new model default in run.

* Zwischen commit

* remove cross-val - only a graphic test still missing

* Rename tasks and make the option to calibrate eith multiple views available

* Update docs and some other tweaks

* Update baseline and pep8, add cross-validation script

* Fix benchmarks

* What's new, update baselines

@matthiasdusch matthiasdusch referenced this pull request Nov 13, 2018

Closed

WIP: Allow individual glaciers to merge together #600

0 of 5 tasks complete

@matthiasdusch matthiasdusch referenced this pull request Dec 10, 2018

Merged

Allow individual glaciers to merge together #624

7 of 7 tasks complete
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.