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

Log-scale layout of heatmap bricks for log-scale axes (fixes #5991) #6819

Merged
merged 11 commits into from
Jan 26, 2024

Conversation

andrew-matteson
Copy link
Contributor

This fixes the heatmap brick layout identified in #5991 . The logic of make_bounds_array is extended with logic that replicates the linear scale logic in log space.

This PR fixes two problems in my use of the library:

  1. Log-scale heatmaps would not render blocks that calculate a bound < 0.
  2. Log-scale heatmaps would not have evenly sized blocks.

I've added a mock and baseline that behave correctly.

@andrew-matteson
Copy link
Contributor Author

Bumping this @archmoj after the winter holiday. I'm hoping to get this into a near-term release for my team.

@andrew-matteson
Copy link
Contributor Author

@archmoj Can you let me know anything that needs to happen to get this merged?

@archmoj archmoj added bug something broken community community contribution status: reviewable labels Jan 17, 2024
@archmoj
Copy link
Contributor

archmoj commented Jan 17, 2024

Thanks for the fix. 🥇
Looking very good to me.
Over to @alexcjohnson

@alexcjohnson
Copy link
Collaborator

@archmoj after my two comments above this looks good. Thanks @andrew-matteson!

There's still an issue with histogram2d failing on log axes if the bins go past zero, but that's a trickier problem, we need to decide if the bins should also be spaced geometrically (and if so support binning that way) but that feels more like a new feature than a fix like the rest of this PR, since it will alter the data to be displayed, not just the details of how it's displayed - so it doesn't belong here - and is probably a much lower priority anyway, I don't think histogram2d gets a whole lot of use.

@andrew-matteson
Copy link
Contributor Author

@alexcjohnson I think my fixes cover your cases. LMK if you have further thoughts.

@andrew-matteson
Copy link
Contributor Author

There is a webgl-jasmine failure that I don't think is related to the recent changes. If it is, could you let me know what's going on? I can't quite deduce from the error messages.

@andrew-matteson
Copy link
Contributor Author

@alexcjohnson I got the tests to rerun and they passed. LMK if this needs anything else.

CHANGELOG.md Outdated Show resolved Hide resolved
@andrew-matteson
Copy link
Contributor Author

@archmoj @alexcjohnson I'd love to get this in for an upcoming release my team has. Is there anything I need to do to wrap this up?

@archmoj
Copy link
Contributor

archmoj commented Jan 26, 2024

Thanks very much for the fix.
💃
We are planning to have a minor plotly.js release next week which would include this.

@archmoj archmoj merged commit 259abab into plotly:master Jan 26, 2024
1 check passed
@andrew-matteson
Copy link
Contributor Author

Thank you! I appreciate this product so much :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken community community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants