search

pangeo-data / scikit-downscale

Statistical climate downscaling in Python
https://scikit-downscale.readthedocs.io/en/latest/
Apache License 2.0
185 stars 47 forks source link

Added zscore.py #10

Closed jukent closed 4 years ago

jukent commented 5 years ago

Still need to add testing capabilities.

jukent commented 4 years ago

@jhamman Since both pull requests are failing the same tests -- are these failures from something outside of my push?

andersy005 commented 4 years ago

@jukent,

I fixed the root cause for the failing tests in #11. Once it's merged, this PR should pass the tests.

jukent commented 4 years ago

@jukent,

I fixed the root cause for the failing tests in #11. Once it's merged, this PR should pass the tests.

Thank you so much!

jukent commented 4 years ago

I corrected the fit method (there was an error in how I was finding the standard deviation for each day of year across all years). I don't think the failing tests are related to zscore.py.

jhamman commented 4 years ago

@jukent - can you update this branch with the latest master? Then we'll take a look at the CI again.

jhamman commented 4 years ago

Also, thanks for staying on this. I hope to return to this project after AGU.

jukent commented 4 years ago

https://github.com/jhamman/xsd/blob/2bc063f8d34c88a5dd152619ea48ed9d3286d0a0/xsd/pointwise_models/gard.py#L90-L95

n.analogs is set to 200 which is longer than the length of X (100), which means the last 100 distances produced by the kdtree_.query method are infinite, causing the error.

Is there a reason n.analogs is initially set as 200? Should it be set at max to the length of the array?

Good luck at AGU!

jukent commented 4 years ago

I adjusted _predict_one_step within gard.py:

        # get analogs
        kmax = max(len(self.kdtree_.data), self.n_analogs)
        _, inds = self.kdtree_.query(X, k=kmax, **self.query_kwargs)

so that k cannot exceed the length of kdtree_.data.

Now the checks pass, but let me know if you have any concerns about this method.

jhamman commented 4 years ago

@jukent - this is looking great. We need some tests with some dummy data before merging but I'm otherwise happy with where things are.

jhamman commented 4 years ago

@jukent - I've opened a PR for discussion on your fork: https://github.com/jukent/xsd/pull/2