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 defaults to add_missing_bounds #569

Merged
merged 2 commits into from
Nov 27, 2023
Merged

Conversation

acordonez
Copy link
Contributor

@acordonez acordonez commented Nov 27, 2023

Description

This PR adds default axes to the add_missing_bounds() function. If no axes are provided, the dataset will have X, Y, and T bounds added.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

If applicable:

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass with my changes (locally and CI/CD build)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

test add_missing_bounds default

fix default bounds test
Copy link
Collaborator

@tomvothecoder tomvothecoder left a comment

Choose a reason for hiding this comment

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

Thank you for addressing this Ana! I had one minor suggestion. Everything else looks good to me.

It looks like the coverage report uploading step in the GitHub Actions build is failing. I'll try to figure out why this is happening. Once the build passes, you can merge.

xcdat/bounds.py Outdated Show resolved Hide resolved
Co-authored-by: Tom Vo <tomvothecoder@gmail.com>
Copy link

codecov bot commented Nov 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c83f46e) 100.00% compared to head (400cebf) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #569   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines         1605      1605           
=========================================
  Hits          1605      1605           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@acordonez
Copy link
Contributor Author

@tomvothecoder Thanks for your quick review. It look like I don't have write access to merge this.

@tomvothecoder
Copy link
Collaborator

No problem! I'll merge this PR for you.

I added you as an xCDAT org member with write access. You'll be able to work with the main repo directly instead of using your fork, which should be more convenient next time.

@tomvothecoder tomvothecoder merged commit 577d7b6 into xCDAT:main Nov 27, 2023
9 checks passed
@tomvothecoder
Copy link
Collaborator

tomvothecoder commented Nov 27, 2023

Also FYI that I'll need to do another xCDAT release to have this fix publicly available. I'll look into it within the next two weeks.

If I recall correctly, PCMDI Metrics developers sometimes use the local xCDAT build from the main branch to get access to the latest fixes/features. It's not recommended, but can be a temporary workaround.

@lee1043
Copy link
Collaborator

lee1043 commented Nov 28, 2023

@acordonez thank you for working on this.

If I recall correctly, PCMDI Metrics developers sometimes use the local xCDAT build from the main branch to get access to the latest fixes/features. It's not recommended, but can be a temporary workaround.

@tomvothecoder that is true, we can work with the latest version internally but PMP's new version release will be on hold until we get the latest xCDAT with this fix being incorporated. 2 weeks seems fine with us.

However, I am not sure how obs4MIPs folks are using xCDAT -- they might rely on official releases. As this fix also affects obs4MIPs, I think merging it ASAP would be appreciated. @gleckler1 Did I get this correct?

@gleckler1
Copy link

obs4MIPs is in high gear, but still prototyping - previously we've modified our codes to deal with past changes to add_missing_bounds but if things are stable now so will be the obs4MIPs processing codes. I typically get xcdat from conda forge creating an env with it and cmor. @manaster may want to chime in here..

@manaster
Copy link

@gleckler1 this is what I do in regards to xCDAT (create and environment with xCDAT from conda forge, so I think that's the official release?). This proposed fix would seems be great for those of us working on obs4MIPs (at least in terms of our prototype code)! We were thinking we would have to make some changes to our prototype codes based on changes made to 'add_missing_bounds()' between xCDAT version 0.3.3 and xCDAT version 0.6.0 (the most recent version Peter and I have been using). The fix proposed here would seem to negate the need for those code changes as far as I can tell.

@tomvothecoder
Copy link
Collaborator

@lee1043 @gleckler1 @manaster Got it. I'll draft up a changelog for v0.6.1 and see what other small changes can go in it before releasing within the next week or two.

Tagging @xCDAT/core-developers to make everyone aware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Feature]: Add default value for axes argument in add_missing_bounds()
5 participants