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

feat(vis-type: scatter): Add log scale option for axes #602

Merged
merged 11 commits into from
Nov 5, 2024
Merged

Conversation

dvmoritzschoefl
Copy link
Contributor

@dvmoritzschoefl dvmoritzschoefl commented Oct 28, 2024

Closes #571

The interface allows to pass the two options

  • xAxisType
  • yAxisType

to either support linear or logarithmic scales. Note that only values > 0 are supported (since negative logs are not defined)

image

@dvmoritzschoefl dvmoritzschoefl requested a review from a team as a code owner October 28, 2024 14:36
@thinkh thinkh changed the title general-vis(scatter): Added the option to scale an axis logarithmically feat(vis-type: scatter): Add log scale option for axes Oct 28, 2024
Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

@dvmoritzschoefl Thanks for adding this feature. I tested it and in general it works as intended. However, when switching the x-axis to log scale all points are hidden (tooltips still appear), which is not the case for the y-axis. Could you please take a look and harmonize the behavior?

vis-empty-log-x-axis-type.webm

I renamed the label and setting in the interface from xAxisType to xAxisScale to better reflect the functionality.

There are also some type errors that should be resolved:

Type-checking in progress...
ERROR in ./src/vis/scatter/useLayout.tsx:333:51
TS2554: Expected 0 arguments, but got 1.
    331 |
    332 |       for (let i = 0; i < splom.dimensions.length; i++) {
  > 333 |         axes[`xaxis${i > 0 ? i + 1 : ''}`] = axis('x');
        |                                                   ^^^
    334 |         axes[`yaxis${i > 0 ? i + 1 : ''}`] = axis('y');
    335 |       }
    336 |

ERROR in ./src/vis/scatter/useLayout.tsx:334:51
TS2554: Expected 0 arguments, but got 1.
    332 |       for (let i = 0; i < splom.dimensions.length; i++) {
    333 |         axes[`xaxis${i > 0 ? i + 1 : ''}`] = axis('x');
  > 334 |         axes[`yaxis${i > 0 ? i + 1 : ''}`] = axis('y');
        |                                                   ^^^
    335 |       }
    336 |
    337 |       const finalLayout: Partial<PlotlyTypes.Layout> = {

Found 2 errors in 5309 ms.

thinkh
thinkh previously approved these changes Nov 5, 2024
Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

@dvmoritzschoefl and me discussed that plotly doesn't support markers+text for log scales. Hence, we cannot display the labels when applying a log scale. For now we'll keep it like this.

@thinkh thinkh merged commit f2d6b71 into develop Nov 5, 2024
3 of 4 checks passed
@thinkh thinkh deleted the mh/log_scale branch November 5, 2024 20:47
@github-actions github-actions bot mentioned this pull request Nov 6, 2024
thinkh added a commit that referenced this pull request Nov 6, 2024
## What's Changed
* fix(vis-type: bar): grouping with zero values by @dvdanielamoitzi in
#598
* fix(vis-type: bar): add tooltip to clipped axis labels by
@dv-usama-ansari in #599
* fix(vis-type: bar): selection is lost when bar plot is sorted by
@dv-usama-ansari in #604
* feat: use `FastTextMeasure` to get the truncated labels by
@dv-usama-ansari in #606
* fix(vis-type: bar): axis labels when sort state is null by
@dv-usama-ansari in #607
* fix: capturing a screenshot is broken for several visualizations by
@oltionchampari in #608
* feat(icon): Option for descending sort first + no unsorted state by
@dvdanielamoitzi in #609
* feat: Add `onClick` to `usePan` by @dvmoritzschoefl in
#619
* feat: add `get_default_redis_url()` by @dvviktordelev in
#610
* feat(vis-type: scatter): Add log scale option for axes by
@dvmoritzschoefl in #602
* fix: exports in general vis by @dvdanielamoitzi in
#621
* Added `useAnimatedTransform` hook by @dvmoritzschoefl in
#620
* Pinned @swc/core to 1.7.42 by @dvmoritzschoefl in
#622


**Full Changelog**:
v14.1.0...14.2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

general-vis(scatter): Allow changing axis scaling
2 participants