Skip to content

aspectRatio option #837

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

Merged
merged 11 commits into from
Feb 9, 2023
Merged

aspectRatio option #837

merged 11 commits into from
Feb 9, 2023

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Apr 5, 2022

Questions:

  • name? (daspect, dataAspectRatio, height = square)?
  • application to other scale types? (time, utc at least, maybe band scales too?)
  • position? (middle/left/right/top/bottom, top-left etc)?
  • syntax (number, array [x, y], number and position (top-left…)?

closes #836
closes #445

@mbostock
Copy link
Member

mbostock commented Apr 9, 2022

I was thinking something different for this feature, which is that if you specify this option (which I’d probably want to call aspectRatio), it would compute the necessary height based on the given (or default) width and margins. Meaning, we’d compute this in autoHeight rather than in autoScaleRange.

Another thought: if we don’t want to introduce option is that we could have a special named value for the height option, such as height = square for a 1:1 aspect ratio.

@Fil
Copy link
Contributor Author

Fil commented Apr 11, 2022

Agree, this will result in much nicer charts (my approach was creating a lot of whitespace).

@Fil Fil marked this pull request as ready for review May 25, 2022 15:02
@Fil
Copy link
Contributor Author

Fil commented May 25, 2022

the last remaining issue is the name of the option.

  • aspectRatio doesn't mention that we're speaking about the data/scales/channels, but not the chart's aspect ratio
  • daspect sounds awfully technical
  • dataAspectRatio is quite a mouthful
  • height: "square" doesn't connect (in my brain) with this concept

Another element that we might want to address at a later stage is if we want to size r with the same logic as x and y (for example, if x and y represent metric positions, and r an euclidian distance).

@Fil Fil requested review from tophtucker and mbostock May 25, 2022 15:07
@yurivish
Copy link
Contributor

If there is ever a future option able to set the “data width” and “data height” then maybe the mouthful is worth it. To me dataAspectRatio does a good job of capturing the meaning of the option.

@mbostock mbostock changed the title daspect option dataAspectRatio option May 29, 2022
@Fil Fil mentioned this pull request May 31, 2022
@Fil
Copy link
Contributor Author

Fil commented Jul 6, 2022

rebased on main

@Fil
Copy link
Contributor Author

Fil commented Nov 15, 2022

rebased on main
cc: @mootari

@tophtucker
Copy link
Contributor

tophtucker commented Nov 16, 2022

I love this. I was scared to look at the diff but the actual change without tests/docs is 25 lines! 😅 ❤️ Not an expert but looks good and non-invasive.

It's restricted to "utc", "time", and "linear" because those are the only linear scales, and this is only possible for linear scales, right? Might add that to the readme. I like being able to mix "utc"/"time" and "linear" (spacetime diagrams! natural units with aspect ratio of c!). The milliseconds spanned by a temporal x axis usually dwarfs the number of units on the y axis, but it's wonderful if you have relative time in ms on the y axis!

I agree with Yuri that dataAspectRatio suggests the meaning well. I also think aspectRatio would be fine. It's trivial to control the outer frame's pixel-space aspect ratio (width = height), so maybe it's fine to give the shorter name to the nontrivial option? I guess it’s like the aspect ratio of the “grid”, of the plane, of the fabric of reality lol. What does “aspect” mean anyway? It’s the ratio of 1 in x to 1 in y… unitRatio lol. Idk. Whatever! But very valuable feature!

I love the Libor test! Let me know if I can help.

@Fil
Copy link
Contributor Author

Fil commented Nov 16, 2022

Thank you for the enthusiastic support. The feature could be expanded in the future but we're currently constrained to linear scales because of the wording:

a variation of one unit in the x dimension is represented by the same number of pixels
There may be a case for a non-linear dataAspectRatio where x and y share a common domain.

You can totally daspect time (utc) vs duration (milliseconds), see https://observablehq.com/@recifs/dataaspectratio-837

Wording for the README might be: both x and y must be linear (including time scales, for which the unit is the millisecond)?

@Fil Fil mentioned this pull request Dec 7, 2022
@mbostock
Copy link
Member

This would be useful for a lot of raster mark applications, too.

@mbostock mbostock changed the title dataAspectRatio option aspectRatio option Feb 9, 2023
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

I have…

  • merged with main
  • renamed to aspectRatio for conciseness
  • extended to non-linear scale types, including sqrt, log, band, and point
  • added some more tests
  • switched to a hard error rather than a warning on unsupported scale types

The semantics of supporting non-linear scale types is of course ambiguous, but I think it’s nice e.g. with two log scales that [1, 10] and [10, 100] are considered equal-length (i.e., that the x = y identity would be drawn at 45° given an aspectRatio of 1 assuming both scales are the same type). I also think it’s especially nice for ordinal scales.

As for conciseness, I don’t think we would ever offer a non-abstract aspect ratio option, and that in general it makes sense in Plot to default to “thinking in abstract coordinates”. If we did want to offer a screen-space aspect ratio in the future, we could add an aspectRatioMode option or something. In any case, I like short and memorable names.

@mbostock
Copy link
Member

mbostock commented Feb 9, 2023

(I am not sure the treatment is 100% correct with respect to padding in ordinal scales, but it’s probably “close enough for now” and we can improve it later.)

@Fil
Copy link
Contributor Author

Fil commented Feb 9, 2023

Re: padding in ordinal scales, I don't think we'd want the autoheight to change when the user specifies a padding (or outer/inner padding), which means the correct approach is to ignore these and base the computation on their default values, as you do here.

Same story with the round option, whose actual value should be ignored, and we should consider only the default. And that's where the computation is not currently perfect, because the default is true, and rounding does not necessarily result in the same bandwidth values for x and y. This is similar to #1239

@Fil Fil merged commit 516cbe3 into main Feb 9, 2023
@Fil Fil deleted the fil/daspect branch February 9, 2023 08:37
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
Co-authored-by: Mike Bostock <mbostock@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fixed data aspect ratio? x/y aspect ratio
4 participants