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

Fixed issue #95: temporal subset specifier ignored if level selected #116

Merged
merged 7 commits into from
Feb 2, 2021
Merged

Fixed issue #95: temporal subset specifier ignored if level selected #116

merged 7 commits into from
Feb 2, 2021

Conversation

agstephens
Copy link
Collaborator

  • fixed the bug
  • added tests to include all possible permutations:
    • no args (collection only)
    • time only
    • level only
    • bbox only
    • time + level
    • time + bbox
    • level + bbox
    • time + level + bbox

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features)
    • test_subset_4D_data_all_argument_permutations
  • Documentation has been added / updated (for bug fixes / features)
    • not needed
  • HISTORY.rst has been updated (with summary of main changes)
  • bumpversion minor has been called on this branch
  • I have added my relevant user information to AUTHORS.md
  • What kind of change does this PR introduce?:

Bug fix. See issue #95

  • Does this PR introduce a breaking change?:

No breaking changes.

  • Other information::

None

@agstephens
Fixed issue #95: temporal subset specifier ignored if level selected
18bf37c 
- fixed the bug
- added tests to include all possible permutations:
    - no args (collection only)
    - time only
    - level only
    - bbox only
    - time + level
    - time + bbox
    - level + bbox
    - time + level + bbox
@agstephens
Copy link
Collaborator Author

@ellesmith88 please review this one and merge when you are happy. There should be no breaking changes. Only a fix to the issue #95.

@agstephens agstephens mentioned this pull request Feb 1, 2021
Closed
@ellesmith88
Copy link
Collaborator

ellesmith88 commented Feb 1, 2021 •
edited

Looks like xarray v0.16.2 is causing the errors here:
combine_by_coords() now raises an informative error when passing coordinates with differing calendars

Edited: But actually this was released on 30 Nov 2020 so I don't know why it would only start being an issue now.

cftime updated yesterday so that's why

@agstephens
Copy link
Collaborator Author

@ellesmith88: do we need to change something to fix this? Thanks

@ellesmith88
Copy link
Collaborator

@agstephens Just updated to cftime <= 1.3.1 👍

@agstephens
Copy link
Collaborator Author

Thanks 👍

@agstephens
Copy link
Collaborator Author

@ellesmith88 I fixed the linting errors. Please accept this PR.

@ellesmith88 ellesmith88 merged commit 04495ab into master Feb 2, 2021
@ellesmith88 ellesmith88 deleted the fix-subset-time-ignored-if-level-subset branch February 2, 2021 09:44
@agstephens agstephens restored the fix-subset-time-ignored-if-level-subset branch July 22, 2021 07:35
@agstephens agstephens deleted the fix-subset-time-ignored-if-level-subset branch November 1, 2021 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellesmith88 ellesmith88 ellesmith88 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@agstephens @ellesmith88