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

Allow width to be set per box in boxplot #4447

Merged
merged 10 commits into from
Oct 21, 2024

Conversation

EdsterG
Copy link
Contributor

@EdsterG EdsterG commented Oct 4, 2024

Description

Allows width to be set per box. Not sure if this is a bug fix or new feature, classified it as a bug fix since colors can be set per box.

This also fixes boxplot when range is zero. Before this PR nothing would get plotted (because the color vector was empty), after this PR the plot works correctly with zero range.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Added an entry in CHANGELOG.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@EdsterG
Copy link
Contributor Author

EdsterG commented Oct 14, 2024

This PR is good to go on my end. Would like to add a test but it overlaps with #4433. How would you like to coordinate it @ffreyer? Can ether wait for #4433 to get merged, OR if this PR is merged before, #4433 can easily be updated to test width, e.g. WGLMakie.boxplot(fig[1, 1], categories, values, width = categories ./ 3).

@ffreyer
Copy link
Collaborator

ffreyer commented Oct 15, 2024

I'll try to get my test related prs merged in the next couple days. If you want you can copy the test from #4433 and add a modified version here, but I think it's easy enough to modify and recognize errors that I can do it later myself.

@EdsterG
Copy link
Contributor Author

EdsterG commented Oct 17, 2024

Why do the tests fail on scatter? I thought that was fixed and RNG stabilized?

@ffreyer
Copy link
Collaborator

ffreyer commented Oct 17, 2024

The PlotUtils fix changed the RNG to StableRNG so the test was expected to fail. I uploaded a new refimg for #4464 now so new test rus should pass.

@EdsterG
Copy link
Contributor Author

EdsterG commented Oct 17, 2024

Nice, thanks for fixing these test issues and for adding all of the new tests.

@EdsterG
Copy link
Contributor Author

EdsterG commented Oct 21, 2024

@ffreyer I've updated the ref image tests to include variable width per category, just needs an update to the ref images. If I understood correctly, only maintainers can update the ref images?

@ffreyer
Copy link
Collaborator

ffreyer commented Oct 21, 2024

Thanks for updating. I uploaded the new refimages and the remaining failure is unrelated to this pr so I'm merging this.

@ffreyer ffreyer merged commit 49b29b6 into MakieOrg:master Oct 21, 2024
17 of 18 checks passed
@EdsterG EdsterG deleted the fix_boxplot_width branch October 21, 2024 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

3 participants