Adds custom color scale to Marker #710

emilianbold · 2019-11-04T12:33:26Z

Thanks for contributing.

Tick to sign-off your agreement to the Developer Certificate of Origin (DCO) 1.1

Description

Adds MarkerBuilder.addColorScale(double value, int r, int g, int b) and related JS generation code which allows custom (non-palette) color scales.

Testing

Yes.

emilianbold · 2019-11-11T14:39:36Z

Does this project have a mailing list or some other way to talk among developers / users?

This commit seems fine to me and I would merge it myself but it's unclear to me if you are fine with this approach.

benmccann · 2019-11-11T15:46:26Z

I've never been involved with any of the jsplot stuff, so I'll leave this one to you and Larry

emilianbold · 2019-11-11T17:17:50Z

@benmccann I'm a committer now so if it was up to me I'd just push these changes. I just want to make sure you guys are OK with me doing this by myself? I will be careful, of course, not to break existing APIs, etc. and ask for confirmations if it looks complicated. For many smaller things though it would seem faster to just commit.

I just realized GitHub offers no way for committer to talk among eachother. There's no chat, no mailing list. It's only the discussions in the PRs. Surprising, now that I've noticed it.

benmccann · 2019-11-11T17:47:36Z

I'll usually review commits to the core library. I'd ask @lwhite1 what he prefers for the jsplot library since he's been the one maintaining this one

lwhite1 · 2019-11-13T01:58:36Z

@emilianbold It will be weeks-to-months before I'm able to devote any time to this. If @benmccann and @ryancerf are unavailable, I would say use your judgement

ryancerf

I am not super familiar with the Plotly stuff either, but I did my best to give a meaningful review.

ryancerf · 2019-11-13T06:16:31Z

jsplot/src/main/java/tech/tablesaw/plotly/components/Marker.java

@@ -155,6 +163,39 @@ public String asJavascript() {
    return context;
  }

+  static class Color {
+
+    final int r, g, b;


What about adding the colorScaleCustomRange value to the Color class? It looks like colors and values are always added at the same time.

That will allow you to only add one field to the MarkerBuilder (not two).

ryancerf · 2019-11-13T06:17:31Z

jsplot/src/main/java/tech/tablesaw/plotly/components/Marker.java

+      if (i != 0) {
+        sb.append(",\n");
+      }
+      sb.append("['")


If you add value to the color class as mentioned above you could factor this out into a separate method that takes no parameters.

ryancerf · 2019-11-13T06:23:07Z

jsplot/src/main/java/tech/tablesaw/plotly/components/Marker.java

+      }
+      sb.append("['")
+          .append(values.get(i))
+          .append("', 'rgb(")


This would be more concise with String.format. Something like.

String.format("[r(%d,%d,%d)]", c.r, c.g, c.b)

ryancerf · 2019-11-13T06:25:31Z

jsplot/src/main/java/tech/tablesaw/plotly/components/Marker.java

@@ -129,7 +135,9 @@ public String asJavascript() {
    Map<String, Object> context = new HashMap<>();
    context.put("size", size.length == 1 ? size[0] : Utils.dataAsString(size));
    if (colorScalePalette != null) {


If colorScalePalette always takes precedence over colorScaleCustomRange should we mention that in the javadoc somewhere?

ryancerf · 2019-11-13T06:34:34Z

@emilianbold

I'm a committer now so if it was up to me I'd just push these changes. I just want to make sure you guys are OK with me doing this by myself? I will be careful, of course, not to break existing APIs, etc. and ask for confirmations if it looks complicated. For many smaller things though it would seem faster to just commit.

In core (where I have done my work) we generally try to give each other a bit of time to review before submitting(1-2 days). For changes with little API impact in the plotly stuff I think it is fine to take more leeway especially if we are slowing you down and we have already discussed the issue.

For non trivial changes in core please wait for Larry or Ben to review.

benmccann · 2020-01-12T03:50:04Z

jsplot/src/test/java/tech/tablesaw/components/MarkerTest.java

+    assertTrue(
+        x.asJavascript()
+            .contains(
+                "colorscale: [\n"


this test seems a little brittle since it's sensitive to formatting

I've had this PR in my inbox for many days since these minor remarks are, to me, a mountain of work.

I wonder if it's something obvious I'm not seeing about how to make this test less brittle (there are other existing tests sensitive to formatting) or how Jackson would simplify that little method?

The way I see it, to test the output in a non-brittle way would mean to basically parse that generated JS. Which isn't obvious how (Jackson, you say, but note we don't have JSON, we have Javascript there) and it would make the test much more complex.

As for introducing Jackson that's a good idea, but, again, nothing easy to hook into. I have not one but two branches where I try to see how Jackson might look for the JS/JSON output.

This one https://github.com/emilianbold/tablesaw/commits/emi/jackson is from late last year and I was trying to use whole objects to serialize via Jackson. Not bad, but a lot of work.

This other one https://github.com/emilianbold/tablesaw/commits/jackson is from today and I'm using the easier way of writing the Map<String,Object> via Jackson. This seems better, but, of course, there's a lot of work to migrate everything to Jackson.

In conclusion, I don't know how to move this PR forward. I'll make a PR for the Jackson branch but for this one I don't know how to make it better and the code is starting to become stale. I guess I should just drop the PR.

benmccann · 2020-01-12T03:50:41Z

jsplot/src/main/java/tech/tablesaw/plotly/components/Marker.java

+
+  private static String asColorScaleString(List<Double> values, List<Color> colors) {
+    StringBuilder sb = new StringBuilder();
+    sb.append("[\n");


it might be cleaner to use a JSON library like Jackson for this

larshelge · 2020-02-16T12:17:55Z

jsplot/src/test/java/tech/tablesaw/components/MarkerTest.java

+        x.asJavascript()
+            .contains(
+                "colorscale: [\n"
+                    + "['0.0', 'rgb(255,0,0)'],\n"


You might want to look into JSONassert:

https://www.baeldung.com/jsonassert

https://github.com/skyscreamer/JSONassert

lwhite1 · 2020-02-21T15:41:51Z

Does this project have a mailing list or some other way to talk among developers / users?

We have always just used issues and pull requests for conversations. Are they inadequate for some reason?

emilianbold · 2020-02-28T09:55:55Z

We have always just used issues and pull requests for conversations. Are they inadequate for some reason?

As a new committer is seemed slow to use issue / PRs as a way to ask global questions about the project. Such as, how do you usually do merges? Or, who generally "owns" which chunk and which areas need a PR from a committer and which areas would allow me to do a commit directly? Basic on-boarding questions so to say.

benmccann · 2020-12-18T18:29:55Z

@emilianbold is this PR something that you're going to come back to?

emilianbold · 2020-12-19T01:10:53Z

I rather forgot what was going on in this PR but from my long comment I see there the cost/benefit are quite slim on finishing it, unless you introduced some nicer helpers for the kind of testing(?) that was not proper in the PR. I do wish to resume contributing to open source next year... Hope everything is well with you and Happy Holidays.

…

--emi

On Fri, Dec 18, 2020 at 8:30 PM Ben McCann ***@***.***> wrote: @emilianbold <https://github.com/emilianbold> is this PR something that you're going to come back to? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#710 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHSCQSV6IT73LEOSTR4OLTSVONTFANCNFSM4JIS4MMQ> .

benmccann · 2020-12-19T01:48:52Z

I liked the suggestion of using https://github.com/skyscreamer/JSONassert as a helper for testing. That seems quite straightforward and wouldn't require you to make many changes besides just including and calling the library

Hope all is well with you and happy holidays to you too!

Adds custom color scale to Marker

f7d32fb

emilianbold requested a review from benmccann November 11, 2019 14:38

ryancerf reviewed Nov 13, 2019

View reviewed changes

benmccann reviewed Jan 12, 2020

View reviewed changes

benmccann force-pushed the master branch 2 times, most recently from 6baaa1c to 23716aa Compare January 13, 2020 02:51

larshelge reviewed Feb 16, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds custom color scale to Marker #710

Adds custom color scale to Marker #710

emilianbold commented Nov 4, 2019

emilianbold commented Nov 11, 2019

benmccann commented Nov 11, 2019

emilianbold commented Nov 11, 2019

benmccann commented Nov 11, 2019

lwhite1 commented Nov 13, 2019

ryancerf left a comment

ryancerf Nov 13, 2019

ryancerf Nov 13, 2019

ryancerf Nov 13, 2019

ryancerf Nov 13, 2019

ryancerf commented Nov 13, 2019

benmccann Jan 12, 2020

emilianbold Feb 4, 2020 •

edited

Loading

benmccann Jan 12, 2020

larshelge Feb 16, 2020 •

edited

Loading

lwhite1 commented Feb 21, 2020 •

edited

Loading

emilianbold commented Feb 28, 2020

benmccann commented Dec 18, 2020

emilianbold commented Dec 19, 2020 via email

benmccann commented Dec 19, 2020

Adds custom color scale to Marker #710

Are you sure you want to change the base?

Adds custom color scale to Marker #710

Conversation

emilianbold commented Nov 4, 2019

Description

Testing

emilianbold commented Nov 11, 2019

benmccann commented Nov 11, 2019

emilianbold commented Nov 11, 2019

benmccann commented Nov 11, 2019

lwhite1 commented Nov 13, 2019

ryancerf left a comment

Choose a reason for hiding this comment

ryancerf Nov 13, 2019

Choose a reason for hiding this comment

ryancerf Nov 13, 2019

Choose a reason for hiding this comment

ryancerf Nov 13, 2019

Choose a reason for hiding this comment

ryancerf Nov 13, 2019

Choose a reason for hiding this comment

ryancerf commented Nov 13, 2019

benmccann Jan 12, 2020

Choose a reason for hiding this comment

emilianbold Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

benmccann Jan 12, 2020

Choose a reason for hiding this comment

larshelge Feb 16, 2020 • edited Loading

Choose a reason for hiding this comment

lwhite1 commented Feb 21, 2020 • edited Loading

emilianbold commented Feb 28, 2020

benmccann commented Dec 18, 2020

emilianbold commented Dec 19, 2020 via email

benmccann commented Dec 19, 2020

emilianbold Feb 4, 2020 •

edited

Loading

larshelge Feb 16, 2020 •

edited

Loading

lwhite1 commented Feb 21, 2020 •

edited

Loading