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.

Sign up for GitHub

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

Jump to bottom

remove multi verif from classes #453

Merged
merged 19 commits into from
Sep 25, 2020
Merged

remove multi verif from classes #453

merged 19 commits into from
Sep 25, 2020

Conversation

bradyrx
Copy link
Collaborator

@bradyrx bradyrx commented Sep 25, 2020 •
edited
Loading

Description

Removes ability for multiple verifs in the HindcastEnsemble object to clean up code.

Closes #429

To-Do List

  • Check tests
  • Check notebooks/docs reference to this.

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Performance (if you touched existing code run asv to detect performance changes)
  • Refactoring

How Has This Been Tested?

Checklist (while developing)

  • I have added docstrings to all new functions.
  • I have commented my code, particularly in hard-to-understand areas
  • Tests added for pytest, if necessary.
  • I have updated the sphinx documentation, if necessary.
  • Any new functions are added to the API. (See contribution guide)
  • CHANGELOG is updated with reference to this PR.

Pre-Merge Checklist (final steps)

  • I have rebased onto master or develop (wherever I am merging) and dealt with any conflicts.
  • I have squashed commits to a reasonable amount, and force-pushed the squashed commits.

@bradyrx bradyrx self-assigned this Sep 25, 2020
@aaronspring
Copy link
Collaborator

this PR will be a great simplification for the future. looking good so far.

@aaronspring
Copy link
Collaborator

rebased for you

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 25, 2020

https://naciscdn.org is down so cartopy can't download coastlines/features. nvkelso/natural-earth-vector#416. So commenting it out in the decadal hindcast example for now.

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 25, 2020

Ah shit something happened rebasing...

bradyrx and others added 13 commits September 25, 2020 12:18
@bradyrx
remove multi verif from classes
5d14d30
@bradyrx
some test fixes
9590b51
@bradyrx
fix tests
a0f70cf
@bradyrx @aaronspring
Force mandatory kwargs for PerfectModelEnsemble methods. (#436)
5786a44 
* Force mandatory kwargs for PerfectModelEnsemble methods.

* kwargs=None for PM

* adjust tests

* fix all notebooks

* dim=None initialized.dims remove lead as in xr.mean(dim=None)

* rework bootstrap docstring

* update metrics.rst and comparison.rst

* force iterations

* add Metric and Comparison to docs

* update comparisons.rst

Co-authored-by: Aaron Spring <aaronspring@users.noreply.github.com>
Co-authored-by: AS <aaron.spring@mpimet.mpg.de>
@bradyrx
fix test_repr
7ff266e
@bradyrx
fix test_smoothing
202bb89
@bradyrx
fix all tests
b3f789b
@bradyrx
update main notebooks
63ae65f
@bradyrx
fix decadal notebooks
dab2351
@bradyrx
update monseas
ec21960
@bradyrx
fix subseasonal
61a27f1
@bradyrx
update changelog
e49b7d5
@bradyrx
fix bug
14d91ca
@aaronspring
Copy link
Collaborator

What’s the purpose of changing “” to ‘’ and vice versa? Gets too complicated to review

@coveralls
Copy link

coveralls commented Sep 25, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.3%) to 92.999% when pulling 413141d on single_verif into 9391874 on master.

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 25, 2020

What’s the purpose of changing “” to ‘’ and vice versa? Gets too complicated to review

I don't know why that happened. Let me see if I can fix it.

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 25, 2020

Ugh I don't know why it did that and how to fix it. The swaps between " and ' are so annoying in black. not sure how to just force one.

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 25, 2020

Okay @aaronspring that's gone.

@aaronspring
Copy link
Collaborator

I never call black directly. git add; pre-commit; ...

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 25, 2020

I never call black directly. git add; pre-commit; ...

Yeah I was on a different dev environment by accident which didn't have pre-commit. We have a legacy system where pre-commit forces double quotes into single quotes and then we ask black to ignore the single quotes. It's not sustainable. See #458. We won't ever have this problem again lol.

@bradyrx bradyrx requested a review from aaronspring September 25, 2020 20:58
aaronspring
aaronspring approved these changes Sep 25, 2020
View reviewed changes
Copy link
Collaborator

@aaronspring aaronspring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be great and much simpler to work with. Was a good idea to start with but now better without.

@aaronspring
Copy link
Collaborator

This PR has little overlap with the other one. You can merge it if you want

@bradyrx bradyrx merged commit b4309ca into master Sep 25, 2020
@bradyrx bradyrx deleted the single_verif branch September 25, 2020 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronspring aaronspring aaronspring approved these changes

Assignees

@bradyrx bradyrx

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

multiple verifs?
3 participants
@bradyrx @aaronspring @coveralls