-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Multidimensional histogram #5400
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
Open
TomNicholas
wants to merge
14
commits into
pydata:main
Choose a base branch
from
TomNicholas:multidimensional_histogram_take_2
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.
Open
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
6349bda
skeletal API
TomNicholas c8dff54
redirected plt.hist to go via new hist function
TomNicholas a595fad
fixed mistake in init
TomNicholas 8e0b6eb
added hist functions to auto-generated API doc page
TomNicholas f023a2c
added method to Weighted class
TomNicholas 60ed9c1
keep_attrs
TomNicholas 57b2f57
hypothesis tests for chunking patterns
TomNicholas 2ac4f9b
input checking and organising wrapper around xhistogram.core.histogra…
TomNicholas 248b1b6
Some tests of input argument handling
TomNicholas f5024b2
rigorous checking of bins inputs
TomNicholas 80e665a
removed unnecessary method of checking for dask arrays (+ rogue prints)
TomNicholas 5f826bc
range argument and docstring corrections
TomNicholas e20b69b
tests for validation of range kwarg
TomNicholas d276838
attempt at type hinting
TomNicholas 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 hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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.
Maybe you have plans for this already but will this handle 2d-histograms like the mean oxygen example in https://xhistogram.readthedocs.io/en/latest/tutorial.html#Averaging-a-variable?
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.
Yes, it will be able to calculate 2D (or N-D in general) histograms in
xarray.hist()
, it just won't be able to plot them inxarray.plot.hist()
, because you need something like anxarray.plot.imshow()
to plot the results of 2D histogram instead.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.
I'm not sure whether you feel it is at all confusing to use the same word "hist" to refer both to the function that calculates the histogram and to the plotting function?
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.
I think it's fine.
da.hist()
vs.da.plot.hist()
feels distinct enough for me.I think I prefer the functions being called the proper name,
da.histogram()
andda.plot.histogram()
, instead of the shorthand though.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.
Cool.
That would also be marginally easier for the current users of xhistogram, who have been using
xhistogram.histogram
so would only need to change the import.However the two arguments I can see against that are:
matplotlib.pyplot.hist
. Changing that would require a deprecation cycle, but I think with the changes I'm proposing here we could get away without a deprecation cycle (because we're only adding optional arguments, not changing them).cov
,corr
,concat
, it would not be unusually terse.I don't really have a strong opinion though.
Uh oh!
There was an error while loading. Please reload this page.
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.
The deprecation cycle wouldn't hinder your progress since I'm imagining simply just aliasing the method with a warning for a while.
numpy and dask also uses
.histogram()
for their histograms. matplotlib, pandas and hvplot uses.hist()
for the plotting, holoviews uses.Histogram()
.Maybe it's just easier to follow the upstream dependencies decisions then to avoid messing with the muscle memory (even though it's in my opinion "bad" muscle memory)?
And I'm not a fan of those shorthand names either,
cov
id,corr
osion,interp
ret? But I'm fine with just following what upstream are doing, there's something satisfying when you can just jump between the different data structures and everything just works.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.
You make very good points! I think I agree with you now.
🤣