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 test_timeseries #1356

Merged
merged 12 commits into from May 4, 2023
Merged

Add test_timeseries #1356

merged 12 commits into from May 4, 2023

Conversation

juliettelavoie
Copy link
Contributor

@juliettelavoie juliettelavoie commented Apr 28, 2023

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • Tests for the changes have been added (for bug fixes / features)
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • CHANGES.rst has been updated (with summary of main changes)
    • Link to issue (:issue:number) and pull request (:pull:number) has been added

What kind of change does this PR introduce?

  • Add a function test_timeseries. The function takes in an array and the name of a variable (not indicator) and creates a DataArray or Dataset ready to be used for tests.
  • It will also be useful for xscen tests.

Does this PR introduce a breaking change?

no

Other information:

I don't really know how pytest and fixtures work, but I tried something...

  1. @RondeauG @aulemahal Is this what you wanted?
  2. I put it in xclim.testing.helpers as discussed in the meetingthursday, but it seems like it is not found as a fixture. If I try to use it in test_atmos.py, it fails. If I put it in conftest.py, then I am able to use it in test_atmos.py. Should I move it or do something else to make it accessible ?
    Should I do something like line 14 of test_sdba/conftest.py: cannon_2015_dist = pytest.fixture(tu.cannon_2015_dist)?
    -> this is fixed.
  3. If the variable is not recognized, should I put "unknown" in the attrs or just put no attrs ?

@juliettelavoie juliettelavoie marked this pull request as draft April 28, 2023 17:39
xclim/testing/helpers.py Outdated Show resolved Hide resolved
@juliettelavoie juliettelavoie marked this pull request as ready for review May 3, 2023 14:30
tests/test_atmos.py Outdated Show resolved Hide resolved
xclim/testing/helpers.py Outdated Show resolved Hide resolved
xclim/testing/helpers.py Outdated Show resolved Hide resolved
juliettelavoie and others added 2 commits May 3, 2023 11:19
Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
@Zeitsperre Zeitsperre added the enhancement New feature or request label May 3, 2023
@Zeitsperre Zeitsperre added this to the v0.43 milestone May 3, 2023
Copy link
Collaborator

@Zeitsperre Zeitsperre left a comment

Choose a reason for hiding this comment

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

LGTM. Were you planning on replacing the existing timeseries fixtures with this? Might be a lengthy process but I can give you a hand.

@github-actions github-actions bot added the approved Approved for additional tests label May 3, 2023
@juliettelavoie
Copy link
Contributor Author

LGTM. Were you planning on replacing the existing timeseries fixtures with this? Might be a lengthy process but I can give you a hand.

The initial goal of this PR was to create something that xscen could use for tests. It was not my plan to replace existing fixtures, but I/we can do it if it's something that people want/need.

@Zeitsperre
Copy link
Collaborator

@juliettelavoie

Well, how about we get the ball rolling here and see how far we get? I'll start with the tas_series variables.

@juliettelavoie
Copy link
Contributor Author

what about just having the tas_series fixture (and others) call `timeseries ?

@Zeitsperre
Copy link
Collaborator

@aulemahal Just mentioned that as a much nicer approach to me. Let's go with that. I can effect that changes.

@github-actions github-actions bot added the docs Improvements to documenation label May 3, 2023
@Zeitsperre
Copy link
Collaborator

This PR has the @RondeauG of approval

@Zeitsperre Zeitsperre merged commit ba50267 into master May 4, 2023
14 checks passed
@Zeitsperre Zeitsperre deleted the add-test-timeseries branch May 4, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests docs Improvements to documenation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants