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

dedicated histogram plotting function #77

Open
sebhahn opened this issue May 10, 2022 · 3 comments
Open

dedicated histogram plotting function #77

sebhahn opened this issue May 10, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@sebhahn
Copy link

sebhahn commented May 10, 2022

the histogram plot (implemented here) using a custom colorbar should be a dedicated function that can be easily used externally by passing the correct arguments, since it can be quite useful for other (non-map) plots as well

@raphaelquast raphaelquast added the enhancement New feature or request label May 10, 2022
@raphaelquast
Copy link
Owner

raphaelquast commented May 10, 2022

yep, I agree that it should be possible to allow using the "histogram on top of a colorbar" feature independent of EOmaps. (Could be quite useful as an "enhanced colorbar" for arbitrary matplotlib plots.)

However, there are still some open tasks in that feature that would be good to address as well:

  • correctly treat colorbar extension arrows #41
    (they "borrow" space from the colorbar-axis, so the histogram-axis needs to reflect that)
  • make sure all colormaps are properly handled (continuous, discrete, etc.)
    (no actual issues found so far but some tests would be nice to make sure)
  • a "general" function probably needs better control on how to position the axes relative to the parent axes

If I find the time I'll look into this... meanwhile projects are always welcome! 🙂

@ocehugo
Copy link

ocehugo commented May 21, 2024

I agree, but I would add that the default colorbar should be the "expected" colorbar, while the histogram one should be an opt-in feature.

I recently faced a problem using the colorbar where some text labels from the histogram axes kept floating around even when hist_size=None. My expectation was if hist_size is None, no histogram axes is created. However, this is not the case. So I went into the code, and the changes to fix were quite spread out — the current custom class strongly depends on the presence of the histogram axes. There is also some - hopefully unnecessary - complexity handling hist_size. I managed to fix it with several sprinkles of if self.hist_size along the code, which smells flaky.

IMO, one of the first steps here is to detach the requirement of having a histogram axis from the Colorbar class and down the rendering tree. @raphaelquast, do you agree? I got some light changes here but I can probably build on top if this aligns with your view.

@raphaelquast
Copy link
Owner

Hey @ocehugo thanks for following up on this issue!

I agree, but I would add that the default colorbar should be the "expected" colorbar, while the histogram one should be an opt-in feature.

Personally I really like the histogram-based colorbar... but I agree that this is not always desired and that there should be an easy way to get a plain simple colorbar.

If colorbar & histogram are added with a single method, I'd rather keep the existing default for backward compatibility.

However, even though this would be a bigger API change, it might actually be a good idea to actually have 2 dedicated methods, e.g.:

m.add_historgram()   # (potentially colored) histogram
m.add_colorbar()     # plain colorbar

This would make the methods much more concise and simplify fine-grained control of the colorbar/histogram.


There is also some - hopefully unnecessary - complexity handling hist_size. I managed to fix it with several sprinkles of if self.hist_size along the code, which smells flaky.

IMO, one of the first steps here is to detach the requirement of having a histogram axis from the Colorbar class and down the rendering tree

Yes I fully agree!
The current implementation is definitely sub-optimal and has potential for a lot of improvements.
Some of the added complexity most probably stems from integration into the LayoutEditor but they should be easy to adapt if necessary.


I got some light changes here but I can probably build on top if this aligns with your view.

I highly appreciate any contributions from your side that result in a better handling of colorbars and histograms!
If you open a PR with your proposed edits, I'm happy to have a look, discuss implementation details and assist wherever necessary!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants