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

unpin xarray #831

Merged
merged 21 commits into from
Oct 7, 2023
Merged

unpin xarray #831

merged 21 commits into from
Oct 7, 2023

Conversation

mathause
Copy link
Contributor

@mathause mathause commented Oct 3, 2023

Description

After #830 we can check if what happens if we unpin xarray (we can still add a lower pin later)

supports conda-forge/climpred-feedstock#26

To-Do List

xclim in #832

Type of change

  • unpin dependencies

How Has This Been Tested?

  • Tests added for pytest except for xclim

Checklist (while developing)

  • CHANGELOG is updated with reference to this PR.

@mathause
Copy link
Contributor Author

mathause commented Oct 4, 2023

The test failure seems real. It tries to assign coords with length 64 (from self.get_initialized()) to coords with length 61 (res). Note that the observations has 61 time steps (self.get_observations()). That did not raise an error in earlier versions of xarray which seems wrong. I don't know enough about climpred to be sure how to fix this.

  1. Use res.coords["valid_time"] = self.get_observations().coords["time"]
  2. Use
    time_no_nan = self.get_initialized().coords["valid_time"]
    time_no_nan = time_no_nan.sel(valid_time=slice(None, res.coords["valid_time"].size)
    res.coords["valid_time"] = time_no_nan
  3. Something else

The test is

def test_same_verifs_valid_time_no_nan(hindcast_hist_obs_1d):

The failure happens in

res.coords["valid_time"] = self.get_initialized().coords["valid_time"]

@aaronspring
Copy link
Collaborator

I don't know enough about climpred to be sure how to fix this.

Agreed. I might be the only one to fix this.

@aaronspring aaronspring self-assigned this Oct 4, 2023
@aaronspring
Copy link
Collaborator

aaronspring commented Oct 4, 2023

but do I remember what I did 3 years ago???, i.e. during my PhD and before becoming a father? I will need to see whether I find a fix there and actually doubt it initially

@Zeitsperre Zeitsperre mentioned this pull request Oct 4, 2023
7 tasks
@mathause
Copy link
Contributor Author

mathause commented Oct 6, 2023

Fair enough! I think the question is - is res actually the observations in disguise, or is it the self.get_initialized() but shorter? Depending on the answer I guess we can with my suggestion (1) or (2) (and either may be fine until this fails for someone else)

@aaronspring
Copy link
Collaborator

res means result and actually prediction skill. So

res.coords["valid_time"] = self.get_initialized().coords["valid_time"]
is to fix valid_time in the result is NaN/NaT are present.

@aaronspring
Copy link
Collaborator

The test failure seems real. It tries to assign coords with length 64 (from self.get_initialized()) to coords with length 61 (res). Note that the observations has 61 time steps (self.get_observations()). That did not raise an error in earlier versions of xarray which seems wrong. I don't know enough about climpred to be sure how to fix this.

  1. Use res.coords["valid_time"] = self.get_observations().coords["time"]

  2. Use

    time_no_nan = self.get_initialized().coords["valid_time"]
    
    time_no_nan = time_no_nan.sel(valid_time=slice(None, res.coords["valid_time"].size)
    
    res.coords["valid_time"] = time_no_nan
  3. Something else

The test is

def test_same_verifs_valid_time_no_nan(hindcast_hist_obs_1d):

The failure happens in

res.coords["valid_time"] = self.get_initialized().coords["valid_time"]

Rather 2. and not 1. could work

@aaronspring
Copy link
Collaborator

The commit I just did might make some coordinate tests fail but should get rid of that error

climpred/classes.py Outdated Show resolved Hide resolved
climpred/classes.py Outdated Show resolved Hide resolved
@aaronspring
Copy link
Collaborator

But now we get these doctests https://github.com/pangeo-data/climpred/actions/runs/6439605339/job/17487468926?pr=831#step:8:174 which used to have overwritten valid_time. So my fix isn't good...

Differences (unified diff with -expected +actual):
@@ -2,7 +2,7 @@
Dimensions: (init: 61, lead: 10)
Coordinates:
- * init (init) object 1954-01-01 00:00:00 ... 2014-01-01 00:00:00
+ init (init) object 1954-01-01 00:00:00 ... 2014-01-01 00:00:00
* lead (lead) int32 1 2 3 4 5 6 7 8 9 10
- valid_time (lead, init) object 1955-01-01 00:00:00 ... 2024-01-01 00:00:00
+ valid_time (lead, init) object nan nan nan nan nan ... nan nan nan nan nan
skill <U11 'initialized'
Data variables

climpred/classes.py Outdated Show resolved Hide resolved
@aaronspring aaronspring removed their assignment Oct 7, 2023
@aaronspring aaronspring added the dependencies Pull requests that update a dependency file label Oct 7, 2023
climpred/classes.py Outdated Show resolved Hide resolved
climpred/classes.py Outdated Show resolved Hide resolved
@aaronspring
Copy link
Collaborator

aaronspring commented Oct 7, 2023

res.valid_time needs to be overwritten by the join of initialized.valid_time and observations.time if both valid_ times don’t have same size

climpred/classes.py Outdated Show resolved Hide resolved
climpred/classes.py Outdated Show resolved Hide resolved
@aaronspring
Copy link
Collaborator

______________ [doctest] climpred.classes.HindcastEnsemble.verify ______________
2211 each initialization, set dim=[].
2212
2213 >>> HindcastEnsemble.coords # doctest: +ELLIPSIS
2214 Coordinates:
2215 * lead (lead) int32 1 2 3 4 5 6 7 8 9 10
2216 * member (member) int32 1 2 3 4 5 6 7 8 9 10
2217 * init (init) object 1954-01-01 00:00:00 ... 2017-01-01 00:00:00
2218 valid_time (lead, init) object 1955-01-01 00:00:00 ... 2027-01-01 00:00:00
2219 * time (time) object 1955-01-01 00:00:00 ... 2015-01-01 00:00:00
2220 >>> HindcastEnsemble.verify(
Differences (unified diff with -expected +actual):
@@ -2,7 +2,7 @@
Dimensions: (init: 61, lead: 10)
Coordinates:
- * init (init) object 1954-01-01 00:00:00 ... 2014-01-01 00:00:00
+ init (init) object 1954-01-01 00:00:00 ... 2014-01-01 00:00:00
* lead (lead) int32 1 2 3 4 5 6 7 8 9 10
- valid_time (lead, init) object 1955-01-01 00:00:00 ... 2024-01-01 00:00:00
+ valid_time (lead, init) object nan nan nan nan nan ... nan nan nan nan nan
skill <U11 'initialized'

@aaronspring
Copy link
Collaborator

Great. That fixed the doctest.

@aaronspring
Copy link
Collaborator

AttributeError in lead can be easily fixed.
doctests require coord ordering.
only few (4) failing tests remaining

@aaronspring
Copy link
Collaborator

ignoring all failing tests from climpred/tests/test_bias_removal.py to be fixed in #832

…ic for metrics threshold_brier_score and rank_histogram; no idea why lead attrs disappear in these metrics but not in others
@aaronspring
Copy link
Collaborator

fixed all tests expect those dealing with bias (from xclim) locally. hope they succeed in CI

@aaronspring
Copy link
Collaborator

Minimal deps tests pass except for 3 hard to debug segfaults/crashes

I would move forward with this. Agree @mathause ?

@mathause
Copy link
Contributor Author

mathause commented Oct 7, 2023

I would move forward with this. Agree @mathause ?

Thanks a lot! All good for me!

@aaronspring aaronspring merged commit 6ed3446 into pangeo-data:main Oct 7, 2023
15 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants