search

VEuPathDB / web-monorepo

A monorepo that contains all frontend code for VEuPathDB websites
Apache License 2.0
2 stars 0 forks source link

130 lineplot marginal histogram #290

Closed moontrip closed 1 year ago

moontrip commented 1 year ago

This will address https://github.com/VEuPathDB/web-monorepo/issues/130 and https://github.com/VEuPathDB/web-monorepo/issues/131. Screenshots will be added in the following comment: tested many cases and all seem to work properly.

Two new props are added to Lineplot component, showMarginalHistogram and marginalHistogramSize. The former decides to display marginal histogram in the Lineplot Viz and the latter is for the size of marginal histogram that has default value of 0.2 (20 %): Lineplot will take 80 %. Perhaps we need to adjust showMarginalHistogram prop to distinguish Lineplot Viz from Lineplot with marginal histogram Viz.

Though, I am not quite sure where to place marginal histogram, top or bottom: a mockup suggested placing marginal histogram at the bottom of Lineplot, but examples we have seen and discussed in the meeting showed top area: personally, it looks better at top area.

moontrip commented 1 year ago
dmfalke commented 1 year ago

Though, I am not quite sure where to place marginal histogram, top or bottom: a mockup suggested placing marginal histogram at the bottom of Lineplot, but examples we have seen and discussed in the meeting showed top area: personally, it looks better at top area.

I think one reason to put it on the bottom is so that is it closer to the x-axis ticks. Maybe @d-callan or @asizemore can weigh in?

d-callan commented 1 year ago

It looks nice above. Can we duplicate the x-axis at the top of the line plot/ below the histo easily?

moontrip commented 1 year ago

It looks nice above. Can we duplicate the x-axis at the top of the line plot/ below the histo easily?

Thank you for your comment @d-callan 👍 I will try to see if I can add the x-axis.

moontrip commented 1 year ago

@d-callan Here are some test results. Hope this will be useful for making a decision. I have firstly checked a possibility of a tooltip described at https://github.com/VEuPathDB/web-monorepo/issues/151, but what I could achieve so far is to display a vertical line to cover up both plots. Tooltip contents are separately made depending on where a hover is located. Please see below two screenshots. Actually lineplot's tooltip contains the number of count (n).

I have also tested whether we can have xaxis tick labels at the bottom of histogram. I could manage to find a way to do that, but some disadvantages are: a) above tooltip cannot be used with this tick labels. It is because additional xaxis tick labels require additional definition of xaxis, saying xaxis2; b) this xaxis tick labels are slightly off in position as compared to the lineplot's tick labels. It is because basically we have two x-axes, instead of shared single x-axis. I could not find a way to align both axes yet. Anyway please see below screenshots

d-callan commented 1 year ago

Thanks @moontrip. If we can't guarantee that both x axes are aligned then I favor the tooltip w dashed line. I think it's still ok to put the histo above. Nice job 👍

moontrip commented 1 year ago

Thanks @moontrip. If we can't guarantee that both x axes are aligned then I favor the tooltip w dashed line. I think it's still ok to put the histo above. Nice job 👍

@d-callan Thank you for your comment! After testing something more, I have one update concerning the positional difference of tick labels between xaxis and xaxis2, as seen earlier. I did not test many cases yet but such a difference is probably not an issue when drawing the plot via Viz because we have set the axis range: in the storybook examples above, I did not consider the range. Here is an example at Viz with the same data used at the storybook and the tick label positions seem to be identical each other. If you prefer aligned xaxis label, then I will check more cases carefully and implement it in this PR: if this is the case, one additional question is whether you want to have a gap in between lineplot and marginal histogram like below screenshot or without the gap.

marginal histogram xaxis2 label05

d-callan commented 1 year ago

Can we see the regular tooltips? Remind me what they look like if we go for the duplicated x axis?

moontrip commented 1 year ago

Can we see the regular tooltips? Remind me what they look like if we go for the duplicated x axis?

@d-callan Yes, we will get regular tooltips that can be seen at either Histogram Viz (hovering histogram) or Lineplot Viz (hovering lineplot) if we use two individual x-axis. Below is an example for hovering Lineplot Viz's marker.

marginal histogram xaxis2 label07

bobular commented 1 year ago

Reviewing now - but I suggest that linked tooltips be "future growth" i.e. another ticket.

bobular commented 1 year ago

Do we prefer a tiny bit of spacing between the plots? with space: image

original no space: image

d-callan commented 1 year ago

I think the standard tooltip w duplicate x axis is fine, and that we should start without the gap between the two plots and see how much of an issue it really is in practice.

@bobular it's already a second ticket.

moontrip commented 1 year ago

I think the standard tooltip w duplicate x axis is fine, and that we should start without the gap between the two plots and see how much of an issue it really is in practice.

@bobular it's already a second ticket.

@d-callan Sounds good. I will make the change accordingly. 👍

bobular commented 1 year ago

The slightly thicker top line (with no gap) bothers me in the storybook examples, but that doesn't seem to be a problem in the EDA. So I agree - no gap!

How about an axis label for the histogram? "N"? "Counts"? Or maybe N: \n Participants (count)

Edit again: I see we have Count in regular histograms. Seems simple (and short) to have here too.

bobular commented 1 year ago

I have to say, it's a thing of beauty

image

d-callan commented 1 year ago

Marginal plots often don't have an axis label, but I could go either way I guess..

moontrip commented 1 year ago

I have to say, it's a thing of beauty

image

@bobular Thank you for your feedbacks earlier. I will address them. By the way, do you prefer x-axis tick labels for marginal histogram or without them like your screenshot?

bobular commented 1 year ago

Hi @moontrip - no rush - I assume you're not working on VEuPath today.

I prefer no x-axis tick labels.

Some unintended consequences have arisen...

Should the controls section be titled "Main Y-axis controls" now? Should we have logscale mode for the histogram? I am happy with "no" as the answer to both of these, but others may have similar questions.

moontrip commented 1 year ago

Hi @moontrip - no rush - I assume you're not working on VEuPath today.

I prefer no x-axis tick labels.

Some unintended consequences have arisen...

Should the controls section be titled "Main Y-axis controls" now? Should we have logscale mode for the histogram? I am happy with "no" as the answer to both of these, but others may have similar questions.

@bobular yes I am off from VEuPathDB today :) Perhaps your questions are follow-up item that can be addressed later if necessary. By the way, just one more question including @d-callan. Since we will go for single x-axis label at the bottom of lineplot, I wonder if you prefer that unified hovering effect with vertically dotted line as I showed earlier (https://github.com/VEuPathDB/web-monorepo/pull/290#issuecomment-1583719116) or just use general tooltip.

d-callan commented 1 year ago

Yes. If we don't have the axes duplicated I think the dotted line make it much more clear which values in the line plot are associated w a bin in the histo

moontrip commented 1 year ago

@bobular I have addressed your feedbacks. And adjusted hovering content when using unified hovering/tooltip.

Actually I found a bug on Lineplot legend, especially when data have null elements (see below screenshot that was taken from qa site: as you can see checkboxes are all inactive). So it is now fixed in this PR too.

moontrip commented 1 year ago

@dmfalke I have defined showMarginalHistogram at visualizationPlugins (withOptions), https://github.com/VEuPathDB/web-monorepo/pull/290/commits/c3b51c90d04888dafe92c775d95f81891c115c6d. Technically, we don't necessarily do this for general lineplot Viz because I have set the default value as false at LineplotVisualization component if options.showMarginalHistogram is undefined, however, it may be better to explicitly define it for clarity.

I think that we may also use this new viz (line + marginal histogram) for standalone map's Viz. Although I did define showMarginalHistogram in there as well, it would require some additional work as I made a note in there (standaloneVizPlugins.ts).

moontrip commented 1 year ago

@bobular Thank you for your comments 👍 Yes, I disabled lineplot with marginal histogram for at pass app but perhaps we need to enable it for the time being (testing etc.). I will do that. Update: done

moontrip commented 1 year ago

Just FYI, @bobular. I have disabled marginal histogram for pass app for now, as we discussed at the meeting. Thanks again for your reviews and tests 👍