-
Notifications
You must be signed in to change notification settings - Fork 225
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
pygmt.binstats: Add alias "tiling" for "T" #2409
Draft
yvonnefroehlich
wants to merge
15
commits into
main
Choose a base branch
from
add-alias-binstats-T
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
354d2c4
Add alias 'tiling' for 'T' in 'binstats.py'
yvonnefroehlich b8c0f9b
Add basic docstring for 'tiling'
yvonnefroehlich ed41595
Fix higlighting
yvonnefroehlich 230f39c
Add missing word and highlighting
yvonnefroehlich 0d9088d
Add highlighting
yvonnefroehlich c692fb7
Merge branch 'main' into add-alias-binstats-T
yvonnefroehlich 5bff1cf
Add missing mathematical expression
yvonnefroehlich 41c2cfd
Add missing '\' for mathematical expression
yvonnefroehlich a237093
Use correct brackets in mathematical expression
yvonnefroehlich a52d8c2
Fix brackets in mathematical expression
yvonnefroehlich 991e39a
Use singular
yvonnefroehlich 9ed9c90
Merge branch 'main' into add-alias-binstats-T
yvonnefroehlich 68f25d9
Fix alphabetic order (code review)
yvonnefroehlich 6a604f9
Merge branch 'main' into add-alias-binstats-T
yvonnefroehlich d328199
Clearfy (hopefully) formulation
yvonnefroehlich File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Using
tiling="h"
outputs the statistics to a table, which was why this alias was not added in the orginal implementation, see #1652 (comment).The way we've handle functions that output to either a grid or table was discussed in #1536, which is to use a "Two methods in a single Python class" like
pygmt.triangulate
andpygmt.grdhisteq
. Unfortunately, if we follow this style, that would require breaking changes topygmt.binstats
🙂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.
Hmm, so @maxrjones mentioned at #1652 (review) that the flag to output statistics to a table is actually more similar to #896 (which is for
fig.*
plotting functions)? Butgmtbinstats
is not a plotting function, so this is kinda awkward 😅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.
Thanks @weiji14 for this clarification and background information. As
tiling
is already used in the docstring forsearch_radius
I thought that adding the doctstring fortiling
was simply overlooked...I am unsure how to continue with this PR. Should we leave it open or should we have a separate issue for this?
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.
Let's leave the PR open for now, the discussion on whether to turn
pygmt.binstats
into two separate methods (e.g.pygmt.binstats.to_grid
orpygmt.binstats.to_table
) can happen here. Anything more general can also be discussed on the open thread at #1536.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.
As I understand this means that it's also not possible to plot the hexagons outlines (polygons )that are used to calculate the stats but to get the center coordinates and stats values for the corresponding hexagon?
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.
@michaelgrund what is your concrete use-case here? Please note, that hexagonal binning is only available for Cartesian data and that only the y-increment can be given and GMT calculates the x-increment given the geometry (https://docs.generic-mapping-tools.org/latest/gmtbinstats.html#t). Based on the upstream GMT documentation, I feel it is not possible to get the outline of the hexagons directly.
Are you looking for figures like this:
In some cases, one can try to determine the (regular) hexagon based on the center of the hexagon (first two columns of the output file via
outgrid
) and the spacing between the centers in y-direction (value passed tospacing
). The outline can then be plotted (be careful with width and height of the figure) using the centers of the hexagon andstyle="h" + str(determined_diameter_of_hexagon) + "c"
. Additionally, the stats output can be used to color-code the fill of the hexagons (please un-comment the corresponding parts in the code below). However, I feel there are other libraries / packages which are more suitable to generate such a visualization.Code to reproduce the figures shown above:
Test data (Please place it in your work dictionary): test_data_in.txt