small but important fix in the scatterplot matrix

[magland]

I've already deployed this change even before merging into main because the bug was causing the scatterplot matrix to be (n-1)x(n-1) rather than n x n.

[codecov-commenter]

Codecov Report

Merging #70 (1428778) into main (f67b109) will not change coverage. The diff coverage is 0.00%.

@@          Coverage Diff          @@
##            main     #70   +/-   ##
=====================================
  Coverage   7.07%   7.07%           
=====================================
  Files         72      72           
  Lines       5653    5653           
  Branches      78      78           
=====================================
  Hits         400     400           
  Misses      5253    5253           

Flag	Coverage Δ
gui_units	8.72% <0.00%> (ø)
svc_units	2.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/tabs/ScatterplotsTab.tsx	0.00% <0.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jsoules]

I'm not sure I'm following this.

I agree that we shouldn't be displaying scatterplots of any variable against itself--those plots aren't going to be very interesting!

But I don't follow what you mean by the scatterplot matrix being (n-1) x (n-1) instead of n x n; shouldn't it always be (n - 1) x (n - 1)? If we have 2 variables selected, we should show 1 plot; if we have 3 variables selected, there are 2 x 2 potential plots to show (one of which is a transpose of another); 4 selected variables gives 6 unique plots that should fit on a 3x3 grid; etc.

In any case, in the rendering function for the 2d matrix (https://github.com/flatironinstitute/mcmc-monitor/blob/36a0dc53906ae8cffeed95b4d1462a688ccc8220/src/tabs/ScatterplotsTab.tsx#L124-L145 later on in ScatterplotsTab.tsx) we have a three-way conditional: if show, and the variables are different, show the plot; if show and variables are the same, place an empty div; and if not-show, place a spacer. So the effect of the proposed change is to replace an empty div with a spacer div, i.e. push the rendered plots to the right in the screen (see screenshots):

Current:

New:

If anything, the flaw I'm seeing in both of these is that we're sizing the individual plots as though they were an n x n grid when they should be sized like an (n-1) x (n-1) one. I've just put together a branch that makes that change, and I'm opening a PR into this branch for it.

[magland]

I'm not sure I'm following this.

I agree that we shouldn't be displaying scatterplots of any variable against itself--those plots aren't going to be very interesting!

But I don't follow what you mean by the scatterplot matrix being (n-1) x (n-1) instead of n x n; shouldn't it always be (n - 1) x (n - 1)? If we have 2 variables selected, we should show 1 plot; if we have 3 variables selected, there are 2 x 2 potential plots to show (one of which is a transpose of another); 4 selected variables gives 6 unique plots that should fit on a 3x3 grid; etc.

I don't think that's quite right. If you have 3 variables selected, then there are 3 x 3 potential plots to show (not 2 x 2) of which we actually show only 3. Similarly, if 4 are selected then you have 6 unique plots on a 4x4 grid (not 3x3).

But this point is moot, since I believe you are doing the best thing on your other PR. So I'll close this one.

[jsoules]

My branch was merged into this; this one still needs to be merged to main. (Reopening this to preserve the history and then merge it.)

[jsoules]

If you have 3 variables selected, then there are 3 x 3 potential plots to show (not 2 x 2) of which we actually show only 3.

Sorry, I was taking a shortcut there. We're displaying pairs of distinct variables, so each side of the box is always one less than the total number of variables selected--so the resulting plots should automatically fit in a box of (n-1) per side. (I can do a more formal proof if you're not convinced though! :smile:)

[magland]

Got it.