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.

Sign up for GitHub

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

Jump to bottom

Fix colors piracy #12

Merged
merged 2 commits into from
Nov 23, 2024
Merged

Fix colors piracy #12

merged 2 commits into from
Nov 23, 2024
+66 −45

Conversation

jpthiele
Copy link
Contributor

This removes the pirated functions Colors.RGB(...) and uses functionality provided by Colors.jl itself.

Related to WIAS-PDELib/GridVisualize.jl#36

@jpthiele jpthiele mentioned this pull request Nov 20, 2024
Open
@jpthiele jpthiele force-pushed the fix-colors-piracy branch 3 times, most recently from 9694b51 to 1f83f69 Compare November 20, 2024 07:11
@j-fu
Copy link
Member

j-fu commented Nov 20, 2024

Has this been tested with GridVisualize.jl ?

@jpthiele
Copy link
Contributor Author

Not yet, no. I also wasn't sure if more tests would be needed, i.e. with another downstream package.

@jpthiele
Copy link
Contributor Author

@j-fu with WIAS-PDELib/GridVisualize.jl#43 all Makie tests pass.
I am not sure about all the extensions though.
Are their tests included in CI?
If not we should think about how to do this, right?

@j-fu
Copy link
Member

j-fu commented Nov 21, 2024

Well, I was happy to switch from pyplot to CairoMakie for basic CI etc in order to avoid any special setup...

Of course this would be nice to have, with priority list:

  1. Makie
  2. PlutoVista
  3. PyPlot
  4. Plots.

For Plots I am not sure if it has good trimesh handling now.

But given the work involved I wouldn't see this high priority. I usally have a look
on the result of (plotting_multiscene)[https://github.com/WIAS-PDELib/GridVisualize.jl/blob/832bac83e289f91f8b224aee4525ab65fe625b5f/examples/plotting.jl#L210] when I want to see if things are still ok.

All other backend code is experimental, we may state this more clearly.

@jpthiele
Copy link
Contributor Author

Okay, then I'll run the test locally for each extension for this PR.
Just to make sure nothing is broken in any of them regarding colors

@j-fu @jpthiele
Add rgbcolor function replacing the pirated Colors.RGB
a82bd28 
This functionality  may be also used in PlutoVista.jl, so it is
better to keep it here.

Also, in this case I am not sure if we should deprectate the creation
of RGB from a tuple.
@j-fu j-fu merged commit 4187910 into WIAS-PDELib:main Nov 23, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jpthiele @j-fu