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

Adding Heatmap trace implementation #148

Merged
merged 5 commits into from
Jan 3, 2020

Conversation

eric-czech
Copy link
Contributor

Hi @alexarchambault, thank you for your work on this project! I love how you've structured it.

I wanted to get more familiar with it by adding Heatmaps and I think this is a solid start. Let me know if I missed anything, but this PR includes:

  • Additions to Trace.scala for Heatmaps
  • Necessary changes in the argonaut codec class to support new parameters, namely colorscale serialization
  • Examples in the Demo
  • New tests in DocumentationTests to force decoding of all the Plot.ly heatmap examples

I also needed to make a few minor tweaks to the make-ghpages.sh script in order to test that the demos are still being rendered correctly.

Lastly, I also needed to update plotly-documentation to correct some errors in the JS examples. How would you suggest I commit that back to your fork? Here's the patch for the diff: plotly-documentation.patch.txt. There's not much to it beyond a typo and some parameters that are supposed to be on the main layout rather than an axis.

Thanks again!

@eric-czech
Copy link
Contributor Author

eric-czech commented Jan 2, 2020

FYI it looks like everything cleared on Travis except the plotly-documentation fixes I mentioned (not sure how to get them upstream for the build).

Also, I'd like to include a way to propagate nulls but they're being replaced somewhere in the encoding process. This might be better left to a separate PR/issue, but do you know off the top of your head how to make this work?

Heatmap(
    z=Seq(
        Seq(10, null.asInstanceOf[Int]), 
        Seq(10, null.asInstanceOf[Int])
    ),
    x=Seq("Yes", "No"),
    y=Seq("Morning", "Evening"),
).plot()

(The cells on the right should be blank instead of 0, like in https://plot.ly/javascript/heatmaps/#heatmap-with-categorical-axis-labels)

Screen Shot 2020-01-02 at 6 03 39 AM

@alexarchambault
Copy link
Owner

Thanks a lot, that looks neat!

I just pushed a commit (alexarchambault/plotly-documentation@eae136b) in the fork of plotly-documentation used as a submodule here, based on your patch.

Check it out from the plotly-documentation directory, and add the updated submodule from the plotly-scala repo (something like cd plotly-documentation && git fetch origin && git checkout eae136b && cd .. && git add -- plotly-documentation should do). That ought to make the tests pass.

@alexarchambault
Copy link
Owner

alexarchambault commented Jan 2, 2020

About the nulls, I think they could be supported either by adding an extra conversion from Seq[Seq[Option[Int]]] (argonaut renders that as either integers or nulls IIRC), and / or adding a conversion from Seq[Seq[Integer]], which are fine with null values. In the former case, users would pass values like Seq(Seq(None, Some(2), Some(3))). In the latter case, they would pass Seq(Seq[Integer](null, 2, 3)) (explicit [Integer] required).

@eric-czech
Copy link
Contributor Author

Awesome, thanks for committing that! That makes sense with the nulls too -- I may try to tackle that with one of those approaches if it starts to chafe a little more but for now I'll leave this PR as-is. Looks like it's all clear now.

@alexarchambault
Copy link
Owner

Merging, thanks @eric-czech!

@alexarchambault alexarchambault merged commit 1d82e3b into alexarchambault:master Jan 3, 2020
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