Skip to content

Histogram as plot type for clusterplot#88

Closed
marabuuu wants to merge 10 commits intohaesleinhuepf:mainfrom
marabuuu:histogram
Closed

Histogram as plot type for clusterplot#88
marabuuu wants to merge 10 commits intohaesleinhuepf:mainfrom
marabuuu:histogram

Conversation

@marabuuu
Copy link

@marabuuu marabuuu commented Jan 7, 2025

Hi @haesleinhuepf,

This PR closes Issue #56.

I have created a class Plotter as a base class and then class ScatterPlotter and class HistogramPlotter as the subclasses. Therefore, I renamed _scatterplot.py into _plot.py and also renamed the corresponding function from scatterplot into plot.

If you want any modifications, just let me know and I try to include them 🌻

Best,
Mara

@marabuuu
Copy link
Author

@haesleinhuepf 🌻

@haesleinhuepf
Copy link
Owner

Hi @marabuuu ,

thanks for working on this!

I'm afraid your modifications are a bit too invasive for my taste. Most importantly: renaming a function such as scatterplot to plot will break all notebooks out in the wild which used the scatterplot function. Hence, the minimum we need to do is to have a scatterplot function that calls plot under the hood. I would also be ok if the function remains "scatterplot". The fact that it displays a histogram is a special case anyway.

Also, I executed the exampe scatterplot notebook in both the old and the new version. The dual-view plot at the very end looked like this in the old version:

image

It looks like this in the new version:

image

I see some issues:

  1. The error in the top left corner.
  2. There is a new tool bar introduced.
  3. The plot can be resized in the bottom right.
  4. Most importantly, we cannot select an intensity range in the histogram. It's not interactive like the 2D plot.

Did you build 2. and 3. intentionally? Do these come with side-effects, does this collide with other functionality?

Would you mind taking another look? Consider making the changes absolutely minimal to not break workflows of people who used these tools before.

Thanks!

Best,
Robert

@marabuuu
Copy link
Author

Oki thank you for your help @haesleinhuepf ! I will take a look ⭐️

@marabuuu
Copy link
Author

marabuuu commented May 5, 2025

Hi @haesleinhuepf,

I have tried to incorporate all requested changes now. Let me know if something else should be modified. Thank you again for your help!

Best,
Mara

@marabuuu
Copy link
Author

I realised that there is an Issue with this PR, so I will close it for now and reopen it once I fixed it :)

@marabuuu marabuuu closed this Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants