-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add a function for subsampling functionality #141
Conversation
So should we merge this? @adehecq |
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 for not having reviewed this earlier. I just have a few small comments that I feel are to a lesser or higher degree important.
xdem/spatial_tools.py
Outdated
valids = np.argwhere(~mask.flatten()).squeeze() | ||
|
||
# Checks that array and npoints are correct | ||
assert np.ndim(valids) == 1 |
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.
This will raise an AssertionError without explanation. Could one be added? Python's AssertionErrors are inherently a little annoying I think because they don't tell the user anything!!
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.
Good point, but actually, I don't think this error should ever happen. It's just to make sure that the array has been flattened correctly and has the expected shape.
I can write "Something is wrong with array dimension, check input data and shape"
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.
Done in 05f5e40
xdem/spatial_tools.py
Outdated
""" | ||
Randomly subsample a 1D or 2D array by a subsampling factor, taking only non NaN/masked values. | ||
|
||
:param subsample: If < 1, will be considered a fraction of valid pixels to extract. |
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.
If <= 1 (to be consistent with the functionality)
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.
Good catch!
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.
Done in 05f5e40
Is this ready for approval? It looks great now in my book! |
+1 |
Yes, I just need to check that the tests pass and run the linters. |
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.
Not much changed by the linters!
Of course, I naturally write good code! ;-) |
As mentioned in PR #135, a shared subsampling function would be useful.
Done:
To do: