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

Volume Area Scaling (VAS) model from Marzeion et.al., 2012 #662

Merged
merged 42 commits into from May 29, 2019

Conversation

Projects
None yet
3 participants
@oberrauch
Copy link
Contributor

commented Jan 7, 2019

Adding the first version of the original mass balance model and volume area scaling model used by Marzeion et.al., 2012, including an altered calibration method. The test_vascaling.py file contains the corresponding unit tests.

  • Tests added/passed.
  • Fully documented.
  • Entry in whats-new.rst: todo
@pep8speaks

This comment has been minimized.

Copy link

commented Jan 7, 2019

Hello @oberrauch! 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 2019-05-29 12:38:00 UTC
Show resolved Hide resolved oggm/utils.py Outdated

oberrauch added some commits Jan 7, 2019

Adding the first version of the original mass balance model used by M…
…arzeion et.al., 2012, including an altered calibration method. The `test_ben.py` file contains the corresponding unit tests.
- Correcting PEP8 issues, mainly too long lines.
 - Deleting the OGGM tests from the `test_ben.py` file
Tests for the mass balance methods & minor improvements:
- Test for the routine computing the minimal and maximal glacier surface elevation
- Tests for the routines computing the mass balance relevant climate information
- Added `ben_params` as basename in the config file
- Minor changes such as typos and similar
Finalising the mass balance tests
- Test the monthly specific mass balance against the corresponding annual mb
- Test the annual mass balance against the 'manually' computed mass balance
- Add a comparison with the reference mass balance measurements to the spec mb test
Started to work on the dynamic part (end of day commit):
- Added area-volume and area-lenght scaling parameters to `params.cfg`
- First steps on what will become the `BenModel` class
Continued to work on the dynamic model (end of day commit):
- Corrected typo in the `cfg.py`
- `step()` function of dynamic model
- `run_until()` function of dynamic model
Note: I'm well aware that the model does not work properly yet. Qualitative tests on Hintereisferner show not observed area/volume/length changes...
Finalizing the volume/area scaling model.
- Changed the name to VAScaling
- Solved problems with scaling constants
- Solved problem with time scales
Test for the VAS model
- Testing the length, area and volume output against the OGGM

@oberrauch oberrauch force-pushed the oberrauch:ben branch from c793ee2 to 8951fc2 Feb 1, 2019

@oberrauch oberrauch changed the title Ben's Mass Balance Model Volume Area Scaling (VAS) model from Marzeion et.al., 2012 May 14, 2019

@fmaussion

This comment has been minimized.

Copy link
Member

commented May 25, 2019

Hi @oberrauch , I had an overview look: this is awesome! Congrats on the great coding and the tests, this all looks good. Let's aim to get this merged this week.

Can you do a couple of things before I have a look again?

  • Check all the docstrings to follow the numpydoc
  • I agree to the suggestion to change the pre-computed reference lists. Add the oggm_ prefix to the existing keys so that you can name your files in oggm-sample-data with a vas_ prefix. Then, you should be able to use the existing code in cfg.py with a simple loop: with prefix in ['oggm', 'vas'] do:
  • add the @entity_task decorator to your VAS tasks (but just the tasks, so I guess: local_t_star, t_star_from_refmb, find_start_area, run_random_vas_climate. This is necessary for multiprocessing and error handling (among other things).
  • we have to think about the name of the tasks. Currently, they have the same name as the OGGM defaults pendants: it makes sense from a python module perspective, but ideally we would have the VAS tasks listed in the oggm.tasks module alongside the others. The reason of the oggm.tasks module is simply to allow users to run OGGM without having to import or touch any of the core modules. So, for full integration of VAS we need to add a vas_ prefix to all your tasks... This is going to be redundant with your module name but I don't see any other way right now.
  • once this is done, add your tasks to oggm.tasks as well.

When this is done I will have another look, but I'd like to merge this ASAP and re-iterate later when we start using it "in production" and when we will write a doc page for it ;-)

Good job!!!

@oberrauch

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

Hey @fmaussion, thanks for the comments and suggestions.
I'm working on it and will let you know once I'm done.

  • Docstrings to numpydoc
  • Reference list prefix
  • @entity_task decorator
  • Add prefix to tasks
  • Add to oggm.tasks

Cheers

@oberrauch

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

  • we have to think about the name of the tasks. Currently, they have the same name as the OGGM defaults pendants: it makes sense from a python module perspective, but ideally we would have the VAS tasks listed in the oggm.tasks module alongside the others. The reason of the oggm.tasks module is simply to allow users to run OGGM without having to import or touch any of the core modules. So, for full integration of VAS we need to add a vas_ prefix to all your tasks... This is going to be redundant with your module name but I don't see any other way right now.
  • once this is done, add your tasks to oggm.tasks as well.

Hi @fmaussion, I have an idea... Do you think it would work if we import the tasks in the tasks.py file with alias that have the vas_ prefix?!

from oggm.core.vascaling import local_t_star as vas_local_t_star
from oggm.core.vascaling import compute_ref_t_stars as vas_compute_ref_t_stars
...

@fmaussion

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Do you think it would work if we import the tasks in the tasks.py file with alias that have the vas_ prefix?!

Yes I believe it would work - I like the idea, but there are small cons as well (name conflicts possible at he package level with two funcs having the same name), but I don't mind too much

@oberrauch

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

but I don't mind too much

Ok, nice. And if problems arise, we can change it later anyways...

@oberrauch

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Hi @fmaussion. I think now we are ready to merge... unless you have some more suggestions.

oberrauch and others added some commits May 29, 2019

@fmaussion

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Let's get this beast in - congrats @oberrauch , great addition! 🎉

@fmaussion fmaussion merged commit e6ff992 into OGGM:master May 29, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.6%) to 87.317%
Details

@fmaussion fmaussion referenced this pull request May 29, 2019

Open

Document VAS model #787

@oberrauch

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Nice. Feels good, even though there is still a lot to do...

@oberrauch oberrauch deleted the oberrauch:ben branch May 29, 2019

@oberrauch

This comment has been minimized.

Copy link
Owner

commented on 6e7be41 May 29, 2019

Thanks @fmaussion, these are all really good points which I overlooked or did not think about.

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.