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

add attrs from climpred to xrobjects #213

Merged
merged 8 commits into from Aug 6, 2019

Conversation

@aaronspring
Copy link
Collaborator

commented Aug 1, 2019

Description

This adds metadata from climpred to xr.datasets and xr.dataarrays.
Just adds a line in the end of the compute functions to write attrs.

Still Todo:

  • only implemented for _compute_bootstrap and not the wrappers. I will change that once you agree with the PR
  • how to handle the other functions, especially persistence? for bootstrap I just write the attrs on all (skill, pi, high_ci, low_ci)

Pain point while developing:

  • inside the functions we change metric from str to function. in the end of the function the str is not available anymore when needed for this new function. rebuilt this inside the new function. not nice coding from my side there.

Fixes #63

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

pytest

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.

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.
  • I have run make html on the documents to make sure example notebooks still compile.
@aaronspring

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2019

Output:

res  = compute_perfect_model(ds_sm, ds_sm)
<xarray.DataArray (lead: 24)>
array([ 0.969878,  0.934025,  0.860032,  0.737175,  0.633061,  0.508628,
        0.365024,  0.283904,  0.241176,  0.174213,  0.278914,  0.275652,
        0.333553,  0.297689,  0.298649,  0.320842,  0.185411, -0.027803,
       -0.132934,  0.003608,  0.007945,  0.037113, -0.041856, -0.036969],
      dtype=float32)
Coordinates:
  * lead     (lead) int64 1 2 3 4 5 6 7 8 9 10 ... 15 16 17 18 19 20 21 22 23 24
Attributes:
    skill calculated by function:  compute_perfect_model
    number of initializations:     20
    number of members:             8
    metric:                        pearson_r
    comparison:                    m2e
    units:                         [ ]
    date:                          created on 2019-08-01 19:25:42

Further improvements could be:

  • translate climpred labels like pearson_r and m2e with a dict to ACC and ensemble mean vs ...
@coveralls

This comment has been minimized.

Copy link

commented Aug 1, 2019

Coverage Status

Coverage increased (+0.5%) to 85.518% when pulling 8227569 on add_skill_attrs into 15b3deb on master.

@bradyrx

This comment has been minimized.

Copy link
Owner

commented Aug 1, 2019

This looks like a great stat for the metadata. Although it looks like you apply assign_climpred_compute_to_attrs at the end of every function. So couldn't this be implemented as a decorator to make things cleaner? @ahuang11

Our decorators have been used to be run at the beginning of a function, but I also think they can be written to run at the end of a function.

@aaronspring

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2019

no. I need to be inside the functions and use some of the keywords

if 'member' in ds.coords:
skill.attrs['number of members'] = ds.member.size

if 'perfect_model' in function_name:

This comment has been minimized.

Copy link
@aaronspring

aaronspring Aug 1, 2019

Author Collaborator

this part is not really nice. I rely on the _function names. Does anyone see a nicer version to convert back to str?

This comment has been minimized.

Copy link
@ahuang11

ahuang11 Aug 4, 2019

Collaborator

Not too sure what's going on here. You're passing in comparison (e.g. 'm2m'), to parse back comparison (e.g. 'm2m')?

Instead of many if/elif statements, you could just add all the lists up and replace [1:] with .lstrip('_') to be more explicit in what it's doing:

ALL_COMPARISONS = HINDCAST_COMPARISONS + PM_COMPARISONS
comparison = get_comparison_function(comparison, ALL_COMPARISONS).lstrip('_')

This comment has been minimized.

Copy link
@aaronspring

aaronspring Aug 5, 2019

Author Collaborator

get_comparison_function(comparison, ALL_COMPARISONS) returns function therefore I use function.__name__

climpred/utils.py Outdated Show resolved Hide resolved

@aaronspring aaronspring requested a review from bradyrx Aug 1, 2019

@ahuang11
Copy link
Collaborator

left a comment

Will look at this more later

climpred/bootstrap.py Outdated Show resolved Hide resolved
climpred/bootstrap.py Outdated Show resolved Hide resolved
climpred/utils.py Outdated Show resolved Hide resolved
climpred/bootstrap.py Outdated Show resolved Hide resolved
climpred/bootstrap.py Outdated Show resolved Hide resolved
climpred/utils.py Outdated Show resolved Hide resolved
if 'member' in ds.coords:
skill.attrs['number of members'] = ds.member.size

if 'perfect_model' in function_name:

This comment has been minimized.

Copy link
@ahuang11

ahuang11 Aug 4, 2019

Collaborator

Not too sure what's going on here. You're passing in comparison (e.g. 'm2m'), to parse back comparison (e.g. 'm2m')?

Instead of many if/elif statements, you could just add all the lists up and replace [1:] with .lstrip('_') to be more explicit in what it's doing:

ALL_COMPARISONS = HINDCAST_COMPARISONS + PM_COMPARISONS
comparison = get_comparison_function(comparison, ALL_COMPARISONS).lstrip('_')
climpred/utils.py Outdated Show resolved Hide resolved
climpred/utils.py Outdated Show resolved Hide resolved
climpred/utils.py Outdated Show resolved Hide resolved
@bradyrx
Copy link
Owner

left a comment

Various minor changes on commenting/syntax suggested by @ahuang11 and myself.

climpred/bootstrap.py Outdated Show resolved Hide resolved
climpred/bootstrap.py Outdated Show resolved Hide resolved
climpred/bootstrap.py Outdated Show resolved Hide resolved
climpred/prediction.py Outdated Show resolved Hide resolved
climpred/prediction.py Outdated Show resolved Hide resolved
climpred/utils.py Outdated Show resolved Hide resolved
climpred/utils.py Outdated Show resolved Hide resolved
climpred/utils.py Outdated Show resolved Hide resolved
climpred/utils.py Outdated Show resolved Hide resolved
climpred/utils.py Outdated Show resolved Hide resolved
@bradyrx

This comment has been minimized.

Copy link
Owner

commented Aug 5, 2019

@aaronspring, make sure to update CHANGELOG as well here.

@aaronspring aaronspring force-pushed the add_skill_attrs branch from 4533a21 to 11c550d Aug 5, 2019

@bradyrx
Copy link
Owner

left a comment

left some more comments

@bradyrx

This comment has been minimized.

Copy link
Owner

commented Aug 5, 2019

@aaronspring, @ahuang11, this might be a good time to implement the xr.set_options() context wrapper used at esmlab by @andersys005. You can add a decorator to specific functions where we want to maintain attributes and coordinates. This would be good for the compute functions to make sure that the original attributes pass through.

Their context wrapper: https://github.com/NCAR/esmlab/blob/master/esmlab/common_utils.py

In action: https://github.com/NCAR/esmlab/blob/master/esmlab/statistics.py

@aaronspring

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 5, 2019

@aaronspring, @ahuang11, this might be a good time to implement the xr.set_options() context wrapper used at esmlab by @andersys005. You can add a decorator to specific functions where we want to maintain attributes and coordinates. This would be good for the compute functions to make sure that the original attributes pass through.

Their context wrapper: https://github.com/NCAR/esmlab/blob/master/esmlab/common_utils.py

In action: https://github.com/NCAR/esmlab/blob/master/esmlab/statistics.py

I dont see where to implement set_options.

@bradyrx

This comment has been minimized.

Copy link
Owner

commented Aug 5, 2019

I dont see where to implement set_options.

It would get added as a decorator above any functions where we want to keep original attributes. xarray by default discards some attributes it doesn't think are relevant along the way. You'd wrap the compute functions with keep_attrs=True for the decorator. Maybe we don't need this. I'd be happy to implement it in another PR

@aaronspring

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 5, 2019

now I see what you mean. we would use the decorator around the compute_functions I guess? I think I wont implement it now. going home for today.

@aaronspring

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 5, 2019

only implemented for _compute_bootstrap and not the wrappers. I will change that once you agree with the PR

Should I reimplement the functions in the bootstrap wrappers or is it ok like it is now?

@bradyrx

This comment has been minimized.

Copy link
Owner

commented Aug 5, 2019

now I see what you mean. we would use the decorator around the compute_functions I guess? I think I wont implement it now. going home for today.

Yeah don't worry about this. I can add a small PR to do this. You've done plenty with this PR!

Should I reimplement the functions in the bootstrap wrappers or is it ok like it is now?

Whatever you think is best here. I think it's a great start to have them on the compute functions. If it's easy to implement for bootstrap and you'll do it anyway down the line, then you could also add that in before we merge.

@aaronspring

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 6, 2019

changes requested is still on

@aaronspring aaronspring requested a review from bradyrx Aug 6, 2019

@aaronspring

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 6, 2019

Should I reimplement the functions in the bootstrap wrappers or is it ok like it is now?

Whatever you think is best here. I think it's a great start to have them on the compute functions. If it's easy to implement for bootstrap and you'll do it anyway down the line, then you could also add that in before we merge.

If I put assign_attrs into the wrapper then I need to change the sig levels into ci_high and _low. also for future changes it will be easier to leave it as is

@bradyrx
bradyrx approved these changes Aug 6, 2019
@bradyrx

This comment has been minimized.

Copy link
Owner

commented Aug 6, 2019

Awesome @aaronspring , thanks for leading this effort. This is a nice addition.

@bradyrx bradyrx merged commit fb1b13f into master Aug 6, 2019

5 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.5%) to 85.518%
Details

@aaronspring aaronspring deleted the add_skill_attrs branch Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.