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 sampling in NuthKaab and set better default values for other classes #439

Merged
merged 37 commits into from
Oct 14, 2023

Conversation

rhugonnet
Copy link
Contributor

@rhugonnet rhugonnet commented Sep 28, 2023

This PR fixes:

  • The sampling in NuthKaab that was done a bit too early, where NaNs introduced by slope/aspect could become problematic (somehow the tests were less sensitive to this).
  • Classes such as Deramp that didn't have a default subsample value set for more reasonable computing times, now all changed to 5e5.
  • The randomness of certain tests with a subsample, such as for Tilt.

Additionally, the previous way of installing the base environment for CI (concatenating YML dependencies without the Python version from a script get_yml_env_nopy) was failing on Windows when dependencies were pinned with a <, for example when I tried scipy<1.11.1 to ensure the randomness problem didn't come from there.
Now the script is renamed generate_yml_env_fixed_py, and generates a temporary YML file during CI to feed to conda, which parses all types of pinning without issues on Windows!

I'm leaving scipy<1.11.1, which also makes variogram modelling fail, will reactivate once they release 1.11.4: #430

adehecq
adehecq previously approved these changes Sep 29, 2023
Copy link
Member

@adehecq adehecq left a comment

Choose a reason for hiding this comment

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

Seems good!
The changes in the tests are related to the fact that the automatically generated ddem has changed because of the change in the coregistration subsamplimg?

@adehecq
Copy link
Member

adehecq commented Sep 29, 2023

Some other tests seem to fail though...

@rhugonnet
Copy link
Contributor Author

Thanks! Yes it's the new subsampling that yields different values.

For the test not passing: it looks like the randomness of results is not fixed between different OS again... Joins #310. Will investigate...

@rhugonnet rhugonnet merged commit 1bec388 into GlacioHack:main Oct 14, 2023
12 of 13 checks passed
@rhugonnet rhugonnet deleted the sampling_defaults branch October 14, 2023 20:49
@adehecq
Copy link
Member

adehecq commented Nov 30, 2023

Haha, I like the many different attempts to make it work !! 🤣

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.

2 participants