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

aaron's commit from refactoring PR #105

Merged
merged 6 commits into from May 2, 2019
Merged

Conversation

bradyrx
Copy link
Collaborator

@bradyrx bradyrx commented Apr 30, 2019

@aaronspring, this is a record of the second commit you made to #98. I wanted to have all of the changes for you here to cherry pick, unless you have them on your local update_dims branch. You can just open a new PR and apply these changes to address relative entropy/bootstrapping issues, etc.

@aaronspring
Copy link
Collaborator

aaronspring commented May 1, 2019

  • _m2m in bootstrap_perfect_model 3D, 1D
  • implemented results as output from bootstrap_perfect_model
  • control run doesn't contain all of the init years
    Comment: changed testing control
  • bootstrap_perfect_model breaks with Datasets.
  • Because of the following line in bootstrap_perfect_model, it breaks when any PM metrics are used...
    Comment: only test bootstrap_perfect_model for xskillscore_metrics for now until compute_persistence_pm can take all metrics
  • change dims for rel_ent
  • dummy data for rel_ent to test with treon (dont know how to properly setup treon but works locally)
  • pytest on synthetic small data
  • travis fails because it cant install bottleneck (https://github.com/bradyrx/climpred/pull/106)
  • adapted PM notebook

Open questions:

  • What do we actually want to test?
  • Loading sample_data to test takes too long:
    123 passed, 48 warnings in 193.26 seconds
    • only test 1d ds and da, since additional coords should just go along?
    • restrict lon, lats even more

My vision for bootstrap_perfect_model:

  • now implemented without caring much about control logic (Checking whether something is True or False)
  • now by default arguments, I calcs skill, probability that persistence and uninit beat init and confidence intervals.
  • especially CI takes a while because of the quantile function. Also because if comparison with persistence might not be needed, I have a flag to leave that out. performance-wise. lets discuss this in the next call.
  • output:
results.coords
Coordinates:
  * i        (i) object 'init' 'pers' 'uninit'
    lon      (y, x) float64 161.9 163.6 165.2 166.9 ... -128.7 -127.1 -125.6
    lat      (y, x) float64 14.14 13.96 13.76 13.56 ... -32.44 -32.58 -32.72
  * lead     (lead) int64 1 2 3 4 5
  * results  (results) object 'skill' 'p' 0.025 0.975
  • I still have to rename i. thats the dimension where you can choose among the different forecasts to look at results (skill, p and CI). Hope the output makes sense to you. I like that it even works for multiple variables in a ds.
    Maybe be the CIs should just be called lowCI highCI?

@aaronspring aaronspring self-assigned this May 1, 2019
@bradyrx
Copy link
Collaborator Author

bradyrx commented May 1, 2019

Just updated the Travis stuff so it should work properly in https://github.com/bradyrx/climpred/pull/106. It's now merged into master. So you should be able to run the following on your command line to get Travis to work:

git checkout master
git pull --rebase
git checkout aaron_bootstrap_updates
git rebase master
git push origin aaron_bootstrap_updates

I'll review this once you're done and request it.

@bradyrx bradyrx added this to In progress in To Do List May 1, 2019
@bradyrx
Copy link
Collaborator Author

bradyrx commented May 1, 2019

What do we actually want to test?

We mostly have acceptance tests (https://en.wikipedia.org/wiki/Acceptance_testing) that prove that they run without error and dont give nan results. We only have very few unit tests

I should read up on testing. Acceptance testing seems like a necessity for all main functions as a first pass that they don't break (that's how we discovered that certain comparisons didn't work). Is there a way to smartly test that the results are accurate? I.e., can we have a few basic 1D skill assessments, save out the output as .npy files, and then in testing load them in and compare the results from the functions? So the two tests would be : (1) the function doesn't break + works with Datasets/DataArrays and (2) the function results are accurate.

Loading sample_data to test takes too long:
123 passed, 48 warnings in 193.26 seconds
only test 1d ds and da, since additional coords should just go along? restrict lon, lats even more

I agree. I did that just to make it work for now. I think we could just generate dummy data. But we can use a specific random seed to ensure the results are always the same and thus address point (2) from above.

@aaronspring aaronspring self-requested a review May 2, 2019 11:52
@aaronspring
Copy link
Collaborator

aaronspring commented May 2, 2019

I just self-reviewed because you started this PR. But I am waiting for you to have a look over it.
I updated the list of changes in comment 2.

Further changes needed:

  • implement treon EDIT: fixed

Next PR:

  • more clear strategy on what bootstrap_compute_perfect_model should be doing
  • implement normalized (PM) metrics for persistence forecast

@bradyrx
Copy link
Collaborator Author

bradyrx commented May 2, 2019

Awesome. I'll get back to this in a couple hours -- need to finish grading some homeworks.

@aaronspring
Copy link
Collaborator

CI breaks because of proplot:
ERROR in testing notebooks/object_oriented_demo.ipynb

@bradyrx
Copy link
Collaborator Author

bradyrx commented May 2, 2019

CI breaks because of proplot:
ERROR in testing notebooks/object_oriented_demo.ipynb

Updated setup to include a pip install climpred['graphics'] option that installs proplot and matplotlib. Will see if that passes Travis while I'm looking through the rest of the PR.

@bradyrx
Copy link
Collaborator Author

bradyrx commented May 2, 2019

object-oriented and relative-entropy notebooks are failing on me. Will check on those now.. doesn't look like anything too difficult.

bradyrx and others added 5 commits May 2, 2019 13:11
add treon to travis

PM notebook bootstrap_compute_perfect_model

fix object-oriented demo; check for clashing coordinates in bootstrap

fix relative entropy to work for LENS case
@bradyrx
Copy link
Collaborator Author

bradyrx commented May 2, 2019

Okay finally. Travis CI now works with treon and pytest. Fixed notebooks breaking, etc. Merging this now. Thanks for cleaning this up! Will chat on the phone about next crucial PRs.

@bradyrx bradyrx merged commit a907559 into master May 2, 2019
@bradyrx bradyrx deleted the aaron_bootstrap_updates branch May 2, 2019 19:37
@bradyrx bradyrx moved this from In progress to Done in To Do List May 2, 2019
bradyrx added a commit that referenced this pull request May 18, 2019
Refactoring from @aaronspring to make bootstrapping/relative entropy work with new dimensions. `pytest` and `treon` now work with Travis CI. Additional testing added.

Former-commit-id: a907559
ahuang11 pushed a commit that referenced this pull request May 22, 2019
Refactoring from @aaronspring to make bootstrapping/relative entropy work with new dimensions. `pytest` and `treon` now work with Travis CI. Additional testing added.

Former-commit-id: a907559
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
To Do List
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants