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

Scale.log fixes #2035

Merged
merged 3 commits into from
Jul 11, 2023
Merged

Scale.log fixes #2035

merged 3 commits into from
Jul 11, 2023

Conversation

berekuk
Copy link
Collaborator

@berekuk berekuk commented Jul 11, 2023

Fixes #2031.

Without a given min and max value in the scale, at certain widths, it doesn't show any axis ticks.
Note: This error only appears for me when the dist is wide enough. When it's under ~600px, it looks fine.
Another note: When hovering, I often see no number, just an empty box.

The root cause for this is that d3.scaleLog().ticks() and d3.scaleLog.tickFormat() are special.

Full docs: https://github.com/d3/d3-scale/tree/main#log_ticks

I'll try to explain what's going on.

D3 scales have two methods, .ticks(count) and .tickFormat(count, specifier). First one outputs an array of ticks to render, and second one return a format function that turns values into strings.

There are two modes for .ticks(count) on log scales:

  1. Output powers of base only, if count < number of powers of base that fit in the domain.

Example:

> scaleLog().domain([5,12000]).ticks(3)
[ 10, 100, 1000, 10000 ]
  1. Output all steps between different powers of base, if count is greater than that.

Example:

> scaleLog().domain([5,12000]).ticks(4)
[
     5,    6,     7,    8,    9,   10,
    20,   30,    40,   50,   60,   70,
    80,   90,   100,  200,  300,  400,
   500,  600,   700,  800,  900, 1000,
  2000, 3000,  4000, 5000, 6000, 7000,
  8000, 9000, 10000
]

(I think there might be off-by-one error here in d3's implementation, but it's not very important; d3.ticks() is never guaranteed to be precise)

Since the second mode outputs a lot of ticks, and ticks in [5..10] range are close to each other, tickFormat() returns an empty string for some ticks.

This affects both hover values and actual ticks.

For hovers, there's a special mode for tickFormat, tickFormat(Infinity), that I now use in drawCursorLines.

For tick labels, the problem was that we have our own algorithm for fitting labels in drawAxes, which forces labels to never overlap. But it could be unlucky and pick only the ticks that are empty. (I'm not sure about the details, but skipping empty labels helped.)

@changeset-bot
Copy link

changeset-bot bot commented Jul 11, 2023

⚠️ No Changeset found

Latest commit: c57aed0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Jul 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
quri-hub ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 11, 2023 8:35pm
squiggle-components ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 11, 2023 8:35pm
squiggle-website ✅ Ready (Inspect) Visit Preview Jul 11, 2023 8:35pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
quri-ui ⬜️ Ignored (Inspect) Visit Preview Jul 11, 2023 8:35pm

@berekuk
Copy link
Collaborator Author

berekuk commented Jul 11, 2023

This is an interesting bug, and it won't be easy to fix:
Captura de pantalla 2023-07-11 a la(s) 13 05 19

It's because we convert distributions to pointsets to render them, but pointsets are not scale-aware and pick x points based on PDF.

I guess this means that x.distribution.pointSet(environment) transformation in DistributionsChart is wrong and we should handle this differently, in scale-aware fashion:

  • measure PDF for equidistant points on X scale for symbolic dists
  • show histograms for samplesets?..
  • there's not much we can do for pointsets if their X points are a bad fit for the given scale

@berekuk
Copy link
Collaborator Author

berekuk commented Jul 11, 2023

For min < 0 in Scale.log, I now throw an error in Squiggle.

This also fixes #1798. (I throw an error when min >= max; note: this might break some existing Squiggle models).

@berekuk
Copy link
Collaborator Author

berekuk commented Jul 11, 2023

  /**
  Here, there could be a better error message.
  For example, "Min must be over 0 for log scale."
*/
  invalidWithNumericFn: Plot.numericFn(
    { fn: {|e|e}, xScale: Scale.log({ min: -1, max: 10 }) }
  )

This is now also a global runtime error.

@berekuk
Copy link
Collaborator Author

berekuk commented Jul 11, 2023

For Scale.symlog, the fix is kinda hard and not worth doing it right now; I've filed a separate #2036 for it to do later.

@OAGr
Copy link
Contributor

OAGr commented Jul 11, 2023

Neat, it seems much better.
image

One small issue is that when the dists are very wide, it shows both the orders of magnitude of 1 and of 2, which looks a bit awkward. I imagine this would be a pain to fix though, I probably wouldn't worry about it now.

image

Copy link
Contributor

@OAGr OAGr left a comment

Choose a reason for hiding this comment

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

Seems good! I like the functionality - code generally looks okay, though I don't understand d3 as well.

@OAGr
Copy link
Contributor

OAGr commented Jul 11, 2023

Kudos for figuring this out, seems gnarly.

@berekuk berekuk merged commit d6879a5 into develop Jul 11, 2023
7 checks passed
@berekuk berekuk deleted the log-scale-fixes branch July 11, 2023 21:55
@OAGr
Copy link
Contributor

OAGr commented Jul 14, 2023

This also fixes #1798. (I throw an error when min >= max; note: this might break some existing Squiggle models).

It seems like that issue is not totally fixed - in the case that there's a min but no max.

d2 = Plot.dist({
dist: normal(0,1),
xScale: Scale.linear({ min: 100 })
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Plot formatting Issues with Log scale
2 participants