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

difference mark and shift transform #1896

Merged
merged 55 commits into from
Nov 8, 2023
Merged

difference mark and shift transform #1896

merged 55 commits into from
Nov 8, 2023

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Oct 17, 2023

Fixes #159. Fixes #1799. TODO:

  • differenceY
  • differenceX?
  • shiftX transform for deriving x1x2 from x
  • shiftY transform?
  • find reducer #1914 for comparing two metrics expressed as separate observations
  • fix z collation as described in Support z in the difference mark #1906
  • fix and test the clip option with the difference mark
  • tests
  • documentation

@mbostock mbostock requested a review from Fil October 17, 2023 16:33
Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! A few comments for now.

src/marks/difference.js Outdated Show resolved Hide resolved
src/marks/difference.js Outdated Show resolved Hide resolved
src/marks/difference.js Outdated Show resolved Hide resolved
test/plots/difference.ts Outdated Show resolved Hide resolved
src/marks/difference.js Outdated Show resolved Hide resolved
@mbostock mbostock requested a review from Fil October 17, 2023 23:29
@mbostock
Copy link
Member Author

Now improved! I have an open question as to whether this should support the z channel, and what to do if fill, fillOpacity, stroke, strokeOpacity, or strokeWidth are specified as channels. I’m inclined to say that these are not supported (at least for now — I’m trying to keep things simple). But I also don’t protect against them, and I’m not entirely sure how we would protect against them. What do you think? Perhaps this elucidates why I think requests like #175 would be so difficult to support!

@Fil
Copy link
Contributor

Fil commented Oct 21, 2023

Toyed with this in #1897 in a composite mark; I also worked on the default tip contents, to show y1, y2, and the actual difference = y1-y2 (which is the point of this mark).

I'd like to suggest two changes to the options, for a bit more consistency:

  • use positive and negative, or positiveFill and negativeFill (instead of positiveColor and negativeColor)
  • make positiveOpacity and negativeOpacity default to fillOpacity (instead of opacity)

this way opacity would be the usual overall mark opacity (line + areas), strokeOpacity would be used on the line.

@mbostock mbostock marked this pull request as ready for review October 24, 2023 00:14
@Fil
Copy link
Contributor

Fil commented Oct 24, 2023

For the record, this (correctly) ignores the computed difference value from the default tip which I suggested in #1897. It was in fact a wrong good idea, because the difference mark is showing the visual difference between scaled values, which is not necessarily encoding y2 − y1. (Adding the difference to the default tip would have been correct only if we could assume a linear scale for y.)

Users can add it explicitly with the channels option, e.g.:

channels: {difference: y1.map((d, i) => d - y2[i])},

or d/y2[i] if using a log scale and comparing ratios.

mbostock and others added 2 commits October 24, 2023 09:32
…**positive**, **negative** are the fill colors for their respective difference areas
src/marks/difference.js Outdated Show resolved Hide resolved
@mbostock mbostock enabled auto-merge (squash) November 7, 2023 18:10
@mbostock
Copy link
Member Author

mbostock commented Nov 8, 2023

LGTY @Fil?

Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes! Thanks for fixing the documentation on z

@mbostock mbostock merged commit d17b34f into main Nov 8, 2023
1 check passed
@mbostock mbostock deleted the mbostock/difference branch November 8, 2023 10:43
@Fil Fil mentioned this pull request Nov 8, 2023
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
* difference mark

* fix filtering; opacity options

* remove unused import

* withTip; don’t duplicate channels

* difference as a composite mark

* difference tip

* reuse channels

* more composite marks

* apply clip as render transform

* consolidate code

* aria labels

* organize imports

* fix differenceY1 test

* update tests

* better defaults

* handle ChannelValueSpec

* update test

* memoTuple

* checkpoint docs

* fix differenceY1 test

* tip fixes

* **positiveOpacity**, **negativeOpacity** default to **fillOpacity**; **positive**, **negative** are the fill colors for their respective difference areas

* positiveFill

* another test

* positiveFillOpacity & fix test

* swap [xy][12]; default y1 = 0

* shift option

* another difference example

* z

* simpler marks (no need for two differences)

* inferScaleOrder

* simpler chart

* enhanced group extent; findX sketch

* shift transform

* shift domain hint

* promote stroke to z

* simpler channel domain hint

* more difference docs

* more difference docs

* more documentation

* call next twice (once for the path, once for the clipPath)

* support clip: frame

* document differenceY

* test ordinal difference

* adopt Plot.find

* more docs

---------

Co-authored-by: Philippe Rivière <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants