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

Re-structure spatialstats.py #378

Open
rhugonnet opened this issue Jun 20, 2023 · 3 comments
Open

Re-structure spatialstats.py #378

rhugonnet opened this issue Jun 20, 2023 · 3 comments
Labels
priority Needs to be fixed rapidly

Comments

@rhugonnet
Copy link
Member

The module spatialstats.py is starting to be a bit messy!

My thoughts right now:

  • Move the spatial variogram scikit-gstat wrappers into a stats/spatial.py in GeoUtils,
  • Move the N-D binning from scipy wrappers into a stats/binning.py in GeoUtils,
  • Rename xdem/spatialstats into xdem/uncertainty.py? Move the fitting of a sum of variograms directly to scikit-gstat?

Any thoughts @adehecq @erikmannerfelt?

@rhugonnet
Copy link
Member Author

From @erikmannerfelt's comment in #158:
Should also move _pandas_str_to_interval to misc.py

@adehecq
Copy link
Member

adehecq commented Jun 22, 2023

Good point ! Yes, it can indeed be a bit difficult to find its way through all the functions... Some ideas/comments:

  • I agree that the scikit-gstat wrappers could be moved to a separate (hidden?) submodule
  • I'm also fine having a module named uncertainty.py containing the higher level functions highlighted in the examples. I don't know if I would move the binning into a separate module though. Wouldn't it be a very small submodule?
  • after our discussions in PR Add BiasCorr classes and rename previous coreg.BiasCorr in coreg.VerticalShift #158, and my experience working with these tools yesterday: wouldn't it make sense to also have a binning and parametric option to calculate the heteroscedasticity? I was working on limited point observations, and in that case, the nearest neighbor extrapolation for values outside the domain didn't make so much sense, while a parametric fit would be super useful !
  • I also noticed while going through the documentation that in the API, one cannot get access to the full docstring of functions in spatialstats.py (and other modules), only a summary. This forced me to check in the code directly, which is not so nice for beginners. Why is that?

@rhugonnet
Copy link
Member Author

OK, perfect!
The binning stuff takes quite a bit of space, happy to add parametric fits (I also added a piecewise application of the original binning in spatialstats.py in #158).
Yes, the API of xDEM is not yet ready, we'll need to organize stuff a bit like in GeoUtils.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority Needs to be fixed rapidly
Projects
None yet
Development

No branches or pull requests

2 participants