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

Add log scale to y-axis #1202

Merged
merged 13 commits into from Oct 12, 2023
Merged

Add log scale to y-axis #1202

merged 13 commits into from Oct 12, 2023

Conversation

hughess
Copy link
Member

@hughess hughess commented Sep 21, 2023

Description

Adds a log scale to the y-axis of charts by introducing a logScale prop (boolean).

This does not include a log scale for the x-axis.

Charts this has been added to:

  • Line
  • Area
  • Bar
  • Scatter
  • Bubble
  • Mixed Charts

Charts this has not been added to:

  • Histogram

Possible Additions

  • Error handling for the following situations:
    • Negative values (currently, chart will appear blank when log scale is turned on and negative values are present)
    • 100% stacked charts (produces a strange-looking chart)

Example

<LineChart data={growth} x=month y=value title="Normal Scale"/>
CleanShot 2023-09-21 at 11 30 52@2x
<LineChart data={growth} x=month y=value logScale=true title="Log Scale"/>
CleanShot 2023-09-21 at 11 30 56@2x

Major Questions

  • Should logScale prop be specific to an axis (ylogScale)?
    • I lean towards yes if we plan to offer either of the following:
      • x-axis log scale (xLogScale)
      • Secondary y-axis, where either axis or both could have a log scale (y2logScale)
  • Should this prop be framed as an "axis type" rather than a boolean
    • E.g., yType=log, xType=log, y2Type=log

Checklist

  • For UI or styling changes, I have added a screenshot or gif showing before & after
  • I have added a changeset

@changeset-bot
Copy link

changeset-bot bot commented Sep 21, 2023

🦋 Changeset detected

Latest commit: f8619b0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@evidence-dev/core-components Minor
@evidence-dev/components Minor
@evidence-dev/evidence Major
evidence-test-environment Patch

Not sure what this means? Click here to learn what changesets are.

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

@vercel
Copy link

vercel bot commented Sep 21, 2023

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

Name Status Preview Comments Updated (UTC)
evidence-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 12, 2023 4:20pm

@netlify
Copy link

netlify bot commented Sep 21, 2023

Deploy Preview for evidence-development-workspace ready!

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/evidence-development-workspace/deploys/65283765b12eff5a8fdbbdff
😎 Deploy Preview https://deploy-preview-1202--evidence-development-workspace.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hughess
Copy link
Member Author

hughess commented Sep 25, 2023

After looking at the secondary y-axis PR again (#874), I think we will need to split this out by axis.

so the prop for now would be “yLogScale”, with “y2LogScale” to be added along with secondary axis

@archiewood
Copy link
Member

Definitely want to be able to log independently

@archiewood
Copy link
Member

You could probably call it just yLog and y2Log

@archiewood
Copy link
Member

thinking: should you be able to optionally control the base of the log here?

like rather than show 1, 10, 100, maybe i want to show 2,4,8,16 etc on the axes. Obviously 10 is the default. but in computing / scientific applications 2 or 8 or e bases are not uncommon

I think echarts has a logBase option.

Copy link
Member

@ItsMeBrianD ItsMeBrianD left a comment

Choose a reason for hiding this comment

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

LGTM, we might want to consider starting to use $props or $restProps for the charts soon to make life easier for these passthroughs

@hughess
Copy link
Member Author

hughess commented Oct 1, 2023

@archiewood have added your suggestions: prop is now yLog and have added yLogBase which defaults to 10, but can be overridden.

Going to skip the x-axis log scale for now as it's slightly more involved. Will be easy enough to add in the future

@archiewood
Copy link
Member

archiewood commented Oct 1, 2023

This is perhaps a side note, but certain types of charts seem poorly suited to a log scale.

Notably:

  • anything showing a notional start from zero (barcharts), since log(0) is undefined
  • especially anything stacked (eg stacked bar, area), since it's hard to interpret what the values mean, and relative sizes are distorted between series.
CleanShot 2023-10-01 at 19 34 19@2x

Perhaps log scales only really make sense for Line and Scatter Charts. Should be protect people from accidental traps here, or let them do what they want (there might be some valid cases I can't think of).

@archiewood
Copy link
Member

archiewood commented Oct 1, 2023

@hughess
Copy link
Member Author

hughess commented Oct 2, 2023

I agree in general. I think we should not make it available on stacked charts. I’ll add some error handling for that.

I can see a scenario where you’d use it on a single series bar chart - e.g., plotting Covid cases with a bar per day. I think same for a single series area chart.

@archiewood archiewood linked an issue Oct 2, 2023 that may be closed by this pull request
@hughess
Copy link
Member Author

hughess commented Oct 2, 2023

@archiewood @ItsMeBrianD I think this is ready now - would be great to get a last look at it if you have a chance.

I've added some error handling for multi-series stacked charts, so it'll now display "log axis cannot be used in stacked chart"

@archiewood
Copy link
Member

Looking pretty good. All the min max and base stuff works as expected.

Couple of edge cases we should handle:

  1. Negative values / zero values - these should throw
CleanShot 2023-10-02 at 12 58 10@2x
  1. If you don't specify a y, should you be able to yLog? This error needs to be clearer
CleanShot 2023-10-02 at 13 01 05@2x CleanShot 2023-10-02 at 12 59 04@2x

Is this as expected?

@hughess
Copy link
Member Author

hughess commented Oct 2, 2023

@archiewood thanks.

  1. That makes sense. I think we can easily access the min values in the y columns, so hopefully not to difficult to add that
  2. That's as expected since the y value will be filled in automatically. In this case, there are multiple y columns getting auto-assigned to y, so it's reading that as a multi-series stacked chart and throwing the error.
  3. I'm not sure what's going on here - I can't tell why only one of the series is being displayed

@hughess
Copy link
Member Author

hughess commented Oct 2, 2023

Ok going to call it here. Some edge cases still exist, but I think they are quite small %s.

@hughess hughess merged commit 1530771 into main Oct 12, 2023
13 of 18 checks passed
@hughess hughess deleted the log-scale branch October 12, 2023 18:22
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.

LineChart y-values logarithmic support
3 participants