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

Fix bugs following geoutils update #291

Merged
merged 24 commits into from
Sep 3, 2022

Conversation

adehecq
Copy link
Member

@adehecq adehecq commented Aug 25, 2022

Because of the MemoryFile PR in geoutils, several attributes of Raster objects do not exist. Fix bugs that arise with that change. Things important to keep in mind:

  • the rasterio DatasetReader initially accessed with self.ds was removed
  • the method self._get_rio_attrs was removed
  • the method self._update was removed

Fixed several issues, but I still have a few that fail locally:

  • tests/test_docs.py::TestDocs::test_build -> don't understand why
  • tests/test_fit.py::TestRobustFitting::test_robust_sumsin_fit -> fail with AssertionError @rhugonnet do you want to check?
  • tests/test_spatialstats.py::TestVariogram::test_sample_multirange_variogram_default -> @rhugonnet?
  • xdem/terrain.py::xdem.terrain.slope -> there is some rounding error in the docstring. Not sure how we can set the threshold for doc checks.

@rhugonnet
Copy link
Contributor

rhugonnet commented Aug 25, 2022

Good news: most of these tests you mention don't fail on the CI.
Two reasons:

  • Need to overwrite your example files locally,
  • Need to update your environment (this will surely fix the tests/test_fit.py::TestRobustFitting::test_robust_sumsin_fit, which used to fail randomly but not anymore, see Basinhopping randomly fails in CI #209)

Bad news: the different value for variograms is also an issue in the CI, and I don't understand that one yet.
Edit: I have the source. The variogram test didn't fail locally first. I deleted the Longyearbyen data/ and processed/, and now it fails. Something has changed in how we process the data into a dDEM, in the coreg surely, now need to find it (and better tests for coreg...)
Edit: noticing something weird with the nodata of our examples: NoData Value: -3.4028234663852886e+38 (ah no, apparently that's the right value from gdalinfo)
Ah but, yeah, looks like the ddem of the example files is full of slightly modified nodata:

ddem.data.data[0, 0, 0]
3.4028235e+38
ddem.data.data[0, 0, 0] == ddem.nodata
False

@rhugonnet
Copy link
Contributor

rhugonnet commented Aug 25, 2022

OK, so I added test_example.py with two functions that tests the example data to see if it stays the same. This is in particular to test if the data we process and store as examples (to avoid processing multiple times) stays the same. The data we fetch from xdem-data should never change, but now we'll be sure of it too 😄.

I ran the test changing from geoutils==0.0.8 to after the MemoryFile PR. The values of the arrays were still the same, but the nodata in the ddem array were not. They change from 2316 points with nodata (before MemoryFile PR) to 0 after.

In the examples.py that creates this ddem example file, I noticed the processing with theNuthKaab() was still written old-school (before we changed nodate handling in GeoUtils). Changing it to the current syntax seems to have solved it!

What a run it was to track this one! I hope test_examples.py will prevent this issue in the future 😅

@rhugonnet
Copy link
Contributor

Tests finally passing, in the end it was a rasterio issue with >1.3. Opened GlacioHack/geoutils#293 and we'll have to create a reproducible example to open an issue there as well.

@rhugonnet rhugonnet merged commit f6584eb into GlacioHack:main Sep 3, 2022
@adehecq adehecq deleted the fix_geoutils_update branch October 21, 2022 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants