-
Notifications
You must be signed in to change notification settings - Fork 224
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
Wrap grdhisteq #1433
Wrap grdhisteq #1433
Conversation
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.
Gentle push on this. I'm starting a project using Sentinel 2 data so I imagine this function would come in handy at some point for doing histogram equalized plots 😄
Looking at https://docs.generic-mapping-tools.org/6.2/grdhisteq.html closely, it seems that both grid outputs (-G
) and table outputs (-D
) are possible. Happy to only do grid
outputs for now, but if you want to do table
outputs too, the triangulate
implementation at #731 can be a good resource to follow.
P.S. Recommend to update branch and resolve merge conflict before applying suggested changes below.
I'll try to wrap up the remaining tasks (i.e., polishing docstring, adding table output test) tomorrow. Edit: I will need a bit longer to finish this, since the implementation will depend on the results from GenericMappingTools/gmt#5785. |
I changed the implementation a bit relative to #731 to support ASCII or pandas.DataFrame output for writing out the equalized histogram. Still, the code is a bit clunky in order to support four different output types (pandas.DataFrame, xarray.DataArray, netCDF, or ASCII). What would you think about having two PyGMT functions for GMT's grdhisteq module rather than just one? One function could write out the data ranges of histogram equalization to a pd.DataFrame or ASCII table and the other could write out the cumulative distribution statistics to a netCDF file or xarray.DataArray. I guess coming up with the names for these would be harder than the current implementation, but I think it would be more user friendly long-term. |
Thanks for all your helpful feedback! |
Co-authored-by: Dongdong Tian <[email protected]>
if isinstance(outfile, str) and output_type != "file": | ||
msg = ( | ||
f"Changing 'output_type' from '{output_type}' to 'file' " | ||
"since 'outfile' parameter is set. Please use output_type='file' " | ||
"to silence this warning." | ||
) | ||
warnings.warn(message=msg, category=RuntimeWarning, stacklevel=2) | ||
output_type = "file" |
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.
Perhaps we should raise an exception here rather than give a warning? Sometimes people just ignore warnings.
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.
There was a bit of a discussion about this following this comment #1284 (review). See #1284 (comment) for a specific discussion about raising a warning versus exception. I think we should follow that lead, although we could move this to a helper function to make it more clear that the logic is used for multiple modules.
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Wei Ji <[email protected]> Co-authored-by: Dongdong Tian <[email protected]>
Description of proposed changes
This PR will wrap the gmt module
grdhisteq
.Preview docs at https://pygmt-git-grdhisteq-gmt.vercel.app/api/generated/pygmt.grdhisteq.html
Fixes #1399
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Slash Commands
You can write slash commands (
/command
) in the first line of a comment to performspecific operations. Supported slash commands are:
/format
: automatically format and lint the code/test-gmt-dev
: run full tests on the latest GMT development version