Skip to content

dot default sort: descending R #349

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

Closed
wants to merge 1 commit into from
Closed

dot default sort: descending R #349

wants to merge 1 commit into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented May 1, 2021

it is the most common use case; deactivates as soon as the sort or reverse option is defined

related: #334

@Fil Fil requested a review from mbostock May 1, 2021 04:10
@Fil
Copy link
Contributor Author

Fil commented May 1, 2021

maybe we shouldn't care about the reverse option if it is alone: it means "reverse input order" vs "input order", and since the default is to sort by R independently of the input order, we can ignore it.

@mbostock
Copy link
Member

The problem here is that options.sort === undefined && options.reverse === undefined is not a great test for whether the data is already sorted, since you could have a transform in place which sorts the data (e.g., Plot.sort, but all transforms support the basic sort transform).

Another way of implementing this might be

function dot(data, {x, y, r, ...options} = {}) {
  ([x, y] = maybeTuple(x, y));
  options = {...options, x, y, r};
  if (r != null) options = reverse(sort(options));
  return new Dot(data, options);
}

but again this doesn’t solve the problem of not knowing whether it’s safe to apply the default sort by descending r.

Here are a few alternatives:

  1. Do nothing; always require explicit sort if desired.
  2. Always sort by descending radius; do not allow custom sort orders.
  3. This PR; handles the common case, but may require {...transform({…}), sort: null} to disable.
  4. This PR, but also only sort if options.transform === undefined.
  5. Make a new dedicated option apart from sort and reverse that controls this additional sort behavior (“sorted”?).
  6. Make a new convenience constructor for sorted dots (as shown above).
  7. Change the dot constructors to sort, and offer a convenience constructor for unsorted dots.
  8. Bring back the z channel and default it to r (undoing part of Drop the z channel for bar, dot, link, rect, rule, text, and tick.  #345)?

None of these feel like great options, so I’m probably inclined to do nothing until I feel more confident.

@Fil Fil mentioned this pull request Aug 11, 2021
@Fil
Copy link
Contributor Author

Fil commented Aug 11, 2021

Without this pull-request we would write, for example (penguinSexMassCulmenSpecies) :

      Plot.dot(data, Plot.reverse(Plot.sort("length", Plot.bin({r: "count"}, {
        x: "body_mass_g",
        y: "culmen_length_mm",
        stroke: "species",
        fill: "species",
        fillOpacity: 0.5
      }))))

With a shorthand notation we don't have to compute the reducer a second time (and under a different syntax), and can write:

      Plot.dot(data, Plot.bin({r: "count"}, {
        x: "body_mass_g",
        y: "culmen_length_mm",
        stroke: "species",
        fill: "species",
        fillOpacity: 0.5,
        sortedMark: true
      }))

I have a slight preference for option 5, i.e. having a specific option sortedMark: true for when you want to enable "common sense" sorting at the mark level on the channel R.

As to whether it should be the default, I walk back a little from "it is the most common use case". (For a time series, where dots are piling up by date, we are better served by not sorting.) I still think a nicer default would be sortedMark: true, but it's not such a strong preference between true or false by default.

I'm pushing an opt-in version for the moment, with a default of sortedMark: false.

@mbostock
Copy link
Member

With the sort option for bin #334 we can write:

Plot.dot(data, Plot.bin({
  r: "count",
  sort: "count",
  reverse: true
}, {
  x: "body_mass_g",
  y: "culmen_length_mm",
  stroke: "species",
  fill: "species",
  fillOpacity: 0.5
}))

This is one more line of code that the sortedMark option, but it avoids introducing a new concept (assuming we still want the sort option for bins and groups, but I think we do).

@mbostock mbostock added the question Further information is needed label Aug 11, 2021
@Fil
Copy link
Contributor Author

Fil commented Aug 12, 2021

OK since we already have two options, let's punt on introducing a third one :)

I just want to update penguinSexMassCulmenSpecies to be sorted, as an example. (OK, it was done in 9a4d55a#diff-f64c7bb0ef7cac906348e9d981a71182cb2ce3934e66e11917ab4a32103e49f6)

@Fil Fil closed this Aug 12, 2021
@Fil Fil deleted the fil/sort-dot branch August 12, 2021 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants