-
Notifications
You must be signed in to change notification settings - Fork 10
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
bug: Ml nodata #310
bug: Ml nodata #310
Conversation
setup.cfg
Outdated
@@ -83,9 +83,10 @@ coreg = earthpy | |||
zarr = zarr | |||
numcodecs | |||
ml = dask-ml>=2022.5.27 | |||
scikit-learn>=0.23.0,<=1.2.0 | |||
scikit-learn=1.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can make this scikit-learn>=1.2.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we need to pin that for the moment. It's bc sklearm_xarray is so outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what needs updated in sklearn-xarray
? We are installing a forked version so we can modify it.
I'm a bit hesitant to enforce such a hard dependency pin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment I don't think we have much of an option. My plan is to take a look at this in about 2 weeks when I get done with classes. On a positive note, I think this only comes into play with the machine learning add-ons. So it should n't affect very many users for the moment.
tests/test_open.py
Outdated
@@ -289,6 +290,58 @@ def test_mosaic_mean(self): | |||
) | |||
) | |||
|
|||
def test_union_values(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly confused, because I thought we recently modified this. In main
it is
def test_union_values(self):
filenames = [l8_224077_20200518_B2, l8_224078_20200518_B2]
with gw.open(
filenames,
band_names=['blue'],
mosaic=True,
bounds_by='union',
) as src:
values = src.values[
0,
src.gw.nrows // 2,
src.gw.ncols // 2 : src.gw.ncols // 2 + 10,
]
self.assertTrue(
np.allclose(
values,
np.array(
[
7524,
7538,
7573,
7625,
7683,
7661,
7643,
7773,
7697,
7566,
]
),
)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I think I started the branch right before you pushed the changes. Sorry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay, no worries. I can pull in the updated tests from main
.
I don't have the ability to merge here, assuming you will bundle it with the others |
What is this PR changing?
Checklist