-
Notifications
You must be signed in to change notification settings - Fork 49
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
relax dim requirements #407
Conversation
# set default dim # TODO: should we still do this? | ||
if dim is None: | ||
dim = 'init' if kind == 'hindcast' else ['init', 'member'] | ||
# check allowed dims | ||
if kind == 'hindcast': | ||
is_in_list(dim, ['member', 'init'], 'dim') | ||
elif kind == 'PM': | ||
is_in_list(dim, ['member', 'init', ['init', 'member']], 'dim') | ||
dim = ['init'] if kind == 'hindcast' else ['init', 'member'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could also prevent default keywords for comparison in this PR. should we @bradyrx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just keep this as is for this PR. We can discuss the requirement of dim
for .verify()
funcs. I feel like it's very common to expect a reduction over init
for users, but makes sense to expect them to know metric, alignment, etc. We can talk more about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only half reviewed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patience here. This looks awesome and makes things a lot more flexible.
Description
he.verify(dim=['lon','lat'])
dim
any more, when dimdoesnt
match metric/comparison/dataset.dims (breaking change): fix tests relying on thatdim
are not present inskill
, fore2r
ande2o
member
does not need to be provided asdim
, but still gets reduced inforecast.mean('member')
logical
, needed for doppyo PRsCloses #282
Prepares: doppyo integration
Type of change
Please delete options that are not relevant.
asv
to detect performance changes)How Has This Been Tested?
Please describe the tests that you ran to verify your changes. This could point to a cell in the updated notebooks. Or a snippet of code with accompanying figures here.
Checklist (while developing)
pytest
, if necessary.Pre-Merge Checklist (final steps)
References
Please add any references to manuscripts, textbooks, etc.