Skip to content

Integrating discrete-heatmap2d #4924

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

Closed
nicolaskruchten opened this issue Jun 15, 2020 · 4 comments · Fixed by #4953
Closed

Integrating discrete-heatmap2d #4924

nicolaskruchten opened this issue Jun 15, 2020 · 4 comments · Fixed by #4953
Assignees
Labels
community community contribution feature something new

Comments

@nicolaskruchten
Copy link
Contributor

nicolaskruchten commented Jun 15, 2020

@ordiology has patched gl-heatmap2d to implementing discretization, which could be used to resolve #2924 by implementing zsmooth on heatmapgl (i.e. enabling it to be set to false)... Let's help them prepare a PR to get this functionality exposed in Plotly.js.

@nicolaskruchten nicolaskruchten added this to the v1.55.0 milestone Jun 15, 2020
@archmoj
Copy link
Contributor

archmoj commented Jun 15, 2020

Thaks @ordiology. Would you mind opening a PR from your heatmap-gl fork to this dev-branch?

@archmoj
Copy link
Contributor

archmoj commented Jun 15, 2020

Right now there are conflicts between @ordiology 's gl-heatmap2d and gl-vis repo. Those should be resolved before merging to master. So I thought one could attempt fixing them on a dev branch by adding commits on top, instead of touching @ordiology's original commits. Once this step is completed we canl publish new gl-heatmap2d and could use that to provide zsmooth options in the plotly.js API.

@ordiology
Copy link

Thank you @archmoj ,

I have resolved the conflict (it was the package-lock.json). However, I'm not sure you want to just pull in my branch at this point. The version of the heatmap-gl that I have made is to link with our scientists' platform. I chose to make as minimal changes to the plotly source code as possible to get a discretised heatmap to work for our scientists. I considered renaming my version of the gl-heatmap2d code and creating a new npm package that could be sourced using a separate trace tree in plotly, but I wasn't sure this would be a productive way to implement the option. So, instead, I kept the linking to the gl-heatmap2d through plotly.js exactly the same and changed the manner in which the gl-heatmap2d code operated on the data and simply linked to a local version of my edited gl-heatmap2d rather than the default npm package. This wouldn't work for proper implementation purposes as a zsmooth switch is required to choose between each implementation.

If there's a better way to communicate rather than in github messages, I can send you more information about how I approached the issue and further discuss what would be the simplest way to effect integration of my solution. I'm happy to help any way that I can.

@archmoj
Copy link
Contributor

archmoj commented Jun 16, 2020

@ordiology thanks for the quick fixup.
I don't think we need a separate package. Instead we need to expand gl-heatmap2d to handle both cases (i.e. by passing different options). I am working on other items for a patch release this week. But let's investigate the minimal changes to make that possible.

@archmoj archmoj added feature something new community community contribution labels Jun 24, 2020
@nicolaskruchten nicolaskruchten removed this from the v1.55.0 milestone Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution feature something new
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants