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

allows cycle to be a tuple #87

Merged
merged 18 commits into from
Feb 20, 2025

Conversation

cvanelteren
Copy link
Contributor

Fixes #86

Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Awesome. This needs a test as well.

@cvanelteren cvanelteren requested a review from beckermr February 20, 2025 14:40
@cvanelteren
Copy link
Contributor Author

Pinged a little too early -- it was a bit of a rabbit hole I went into but I think this should be correct.

@cvanelteren
Copy link
Contributor Author

Some tests fail -- particularly on the colormaps. Judging from the actual differences they are minor. I am not exactly sure why they are different. Perhaps @beckermr can spot a mistake with some fresh eyes. Can confirm that the plot on #86 looks at it should now.

@beckermr
Copy link
Collaborator

My guess is that the Cycle object itself is an iterate so you need to move that case statement above the one with Iterable or add a new case statement after the one that catches Cycle objects.

@cvanelteren
Copy link
Contributor Author

Good catch. That fixed it. Results look good now. The failure relates to a missing test and a fixed error.

@cvanelteren
Copy link
Contributor Author

image
image

@beckermr
Copy link
Collaborator

That helped a ton. There is one failure in the box plots that needs investigating.

@beckermr
Copy link
Collaborator

Ah so the new boxplot plot is the correct one?

Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Assuming the new boxplot is correct, Lgtm!

@cvanelteren
Copy link
Contributor Author

New one is correct I think. Thanks will push some styling and then merge it.

@cvanelteren cvanelteren merged commit f21a814 into Ultraplot:main Feb 20, 2025
9 of 12 checks passed
@cvanelteren cvanelteren deleted the allow-tuple-parse-cycle branch February 20, 2025 16:48
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.

Cycle object not parsed correctly as tuple
2 participants