Skip to content

Type trace axis ids#392

Merged
andrei-ng merged 1 commit into
plotly:mainfrom
RedZapdos123:axis-id-enums
Jun 1, 2026
Merged

Type trace axis ids#392
andrei-ng merged 1 commit into
plotly:mainfrom
RedZapdos123:axis-id-enums

Conversation

@RedZapdos123
Copy link
Copy Markdown
Contributor

@RedZapdos123 RedZapdos123 commented Apr 28, 2026

Description:

Issue plotly/plotly.py#5583 points out that trace x_axis and y_axis setters are not self-explanatory because users have to discover values like "x2" and "y3" from examples.

This PR keeps the existing string-based axis ids, but gives them named types:

  • XAxisId
  • YAxisId

The trace fields now use those named axis-id string types, and the trace setters accept impl Into<XAxisId> / impl Into<YAxisId> so existing string inputs continue to work without introducing an enum or a hardcoded axis limit.

It also adds regression tests for both code paths:

  • derive-generated trace setters (Scatter)
  • manual trace setters (Contour)

This PR does not include the CI fixes from #397 which have been moved there unlike earlier.

Checklist:

  • I have reviewed all changes in this PR myself.
  • I have added relevant regression test cases for this fix.
  • I have run formatting checks in WSL using cargo +nightly fmt --all -- --check and cd examples && cargo +nightly fmt --all -- --check.
  • I have run linting checks in WSL using cargo clippy -p plotly -p plotly_derive -- -D warnings -A deprecated.
  • I have run focused tests in WSL using cargo test -p plotly axis_id --features static_export_chromedriver.
  • I have run package tests in WSL using cargo test -p plotly --features plotly_ndarray,plotly_image,static_export_chromedriver,debug.

The validation screenshots of the tests run, locally:

image image

@andrei-ng
Copy link
Copy Markdown
Collaborator

andrei-ng commented May 5, 2026

Hi @RedZapdos123,

Thank you for the PR. And thank you for the work on the CI fixes.

I propose that you make a new PR with the CI fixes to separate it from this PR.

Concerning the original issue, the axis IDs, can you give me a bit more context on why you opened an issue in plotly.py repo and referenced it here ? I didn't and don't understand the connection.

Regarding the fix itself. This crate is trying to follow plotly.py and plotly.js as much as possible. While using an Enum is indeed type safe, clear and idiomatic, it has the downside that it doesn't generalize well for the purpose of plots. There is already a long standing issue in this crate/repo that axis numbers are hardcoded for subplots up to only 8 axes (#184). This is an artificial limit.

So I don't think using an Enum here brings much improvement but rather more complexity and I would prefer not to follow this approach.

@RedZapdos123 RedZapdos123 marked this pull request as draft May 10, 2026 15:19
@RedZapdos123 RedZapdos123 marked this pull request as ready for review May 22, 2026 02:43
@RedZapdos123
Copy link
Copy Markdown
Contributor Author

Hi @RedZapdos123,

Thank you for the PR. And thank you for the work on the CI fixes.

I propose that you make a new PR with the CI fixes to separate it from this PR.

Concerning the original issue, the axis IDs, can you give me a bit more context on why you opened an issue in plotly.py repo and referenced it here ? I didn't and don't understand the connection.

Regarding the fix itself. This crate is trying to follow plotly.py and plotly.js as much as possible. While using an Enum is indeed type safe, clear and idiomatic, it has the downside that it doesn't generalize well for the purpose of plots. There is already a long standing issue in this crate/repo that axis numbers are hardcoded for subplots up to only 8 axes (#184). This is an artifiial limit.

So I don't think using an Enum here brings much improvement but rather more complexity and I would prefer not to follow this approach.

Thank you for the review.

I split the CI work out as requested.

I also reworked the #392 to avoid the enum approach.

The current version keeps the existing string-based axis ids, but gives them named types:

  • XAxisId
  • YAxisId

The trace fields now use those types, and the setters accept impl Into<XAxisId> / impl Into<YAxisId>, so existing string inputs like "x2" and "y3" still work, without introducing a hardcoded axis limit.

I also added regression tests for both paths:

  • derive generated setters (Scatter)
  • manual setters (Contour)

About the 'plotly.py' issue reference. I opened it because the original problem is about the user facing axis-id API feeling unclear from a Plotly user point of view. But I agree that plotly.rs should not use an enum here if that adds an artificial limit, so I changed the implementation to match your feedback.

Signed-off-by: Mridankan Mandal <xerontitan90@gmail.com>
@andrei-ng
Copy link
Copy Markdown
Collaborator

Hi @RedZapdos123,
What would be the benefit of using the axis IDs as you introduced them now? Can you give me some examples, use cases, what would this solve from your (a user's) perspective.

@RedZapdos123
Copy link
Copy Markdown
Contributor Author

Hi @andrei-ng.

A user writes: Scatter::new(x, y).y_axis("y3")

This PR improves is that instead of treating that argument as an unstructured String, the API gives it a named meaning with XAxisId and YAxisId, and the setters consistently accept impl Into<XAxisId> and impl Into<YAxisId> across the trace types.

So, from user (like mine) perspective, the benefit is actually:

  • It makes it clearer that this value is meant to be an axis reference/id
  • It keeps the API open ended.
  • It avoids introducing a hardcoded axis limit.

An good example is subplot usage like this:

let price = Scatter::new(days, prices).y_axis("y");
let volume = Bar::new(days, volumes).y_axis("y2");
let indicator = Scatter::new(days, rsi).y_axis("y3");

What this solves for the users like me, is mainly API clarity. The argument now does not look like just any random string. It is easier to understand from the Rust API that values like "y", "y2" and "y3" are axis references.

@andrei-ng
Copy link
Copy Markdown
Collaborator

Hi @andrei-ng.

A user writes: Scatter::new(x, y).y_axis("y3")

This PR improves is that instead of treating that argument as an unstructured String, the API gives it a named meaning with XAxisId and YAxisId, and the setters consistently accept impl Into<XAxisId> and impl Into<YAxisId> across the trace types.

So, from user (like mine) perspective, the benefit is actually:

  • It makes it clearer that this value is meant to be an axis reference/id
  • It keeps the API open ended.
  • It avoids introducing a hardcoded axis limit.

An good example is subplot usage like this:

let price = Scatter::new(days, prices).y_axis("y");
let volume = Bar::new(days, volumes).y_axis("y2");
let indicator = Scatter::new(days, rsi).y_axis("y3");

What this solves for the users like me, is mainly API clarity. The argument now does not look like just any random string. It is easier to understand from the Rust API that values like "y", "y2" and "y3" are axis references.

Thanks for the reply and for example.

I will merge it and see how the community responds to the new usage.

@andrei-ng andrei-ng merged commit ac7e8c8 into plotly:main Jun 1, 2026
81 of 86 checks passed
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.

3 participants