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

Added sampling without replacement to RandomMassBalance #502

Merged
merged 3 commits into from Jul 13, 2018

Conversation

Projects
None yet
2 participants
@matthiasdusch
Copy link
Member

matthiasdusch commented Jul 9, 2018

Added an option to the RandomMassBalance model:
If without_replacement==False (default option and behavior of RandomMassBalance up till now), every model year an year from the given climate period will be picked, independent on the previous picks.
A 100-year model run with a given 30 year climate period could theoretically use the climate of 1 specific year for 100 consecutive model years.

If without_replacement==True (new option), all years from the given climate period will be used in random order before one year can be picked again.
A 300-year model run with a given 30 year climate will use these 30 years for 10 consecutive times but each time randomly shuffled.
A 100-year model run with a given 100 year climate will use every climate year exactly once, but randomly shuffled compared to the original climate series.

  • no Tests added
  • Documented in massbalance/RandomMassBalance and flowline/run_random_climate
@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Jul 9, 2018

Thanks a lot! This is quite clever, it took me some time to understand the algorithm here ;)

I'm not sure I like the name of the kwarg, but I don't have a brilliant idea either...

How about ensure_unique? It is not strictly correct for periods > window size, but it is still more explicit than the current name. Thoughts?

@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Jul 9, 2018

... and I would welcome some tests - we can discuss a strategy when we meet

@matthiasdusch

This comment has been minimized.

Copy link
Member

matthiasdusch commented Jul 9, 2018

I am not totally convinced by 'without_replacement' either. I just used it in imitation of the classic statistical ballot box experiments with/without replacement.
So I am happy to think about another name.

And I'll come up with some tests.

Edit: If it took you some time to understand the algorithm, it's quite likely I made it unnecessary complicated... :-/

@matthiasdusch

This comment has been minimized.

Copy link
Member

matthiasdusch commented Jul 10, 2018

Updated the pull request:

  • Changed parameter name from without_replacement to unique_samples
  • Added some more comments on the sampling algorithm
  • Added tests: test_models/test_random_mb_unique. Test structure is adapted from test_models/test_random_mb with some adjustments.

@fmaussion fmaussion merged commit bb666ca into OGGM:master Jul 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Jul 13, 2018

Excellent job, thanks a lot!

@matthiasdusch matthiasdusch deleted the matthiasdusch:random2 branch Jul 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment