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

propagate args as-is to convert_arguments(trait, args...) #3859

Merged
merged 1 commit into from
May 14, 2024

Conversation

aplavin
Copy link
Contributor

@aplavin aplavin commented May 12, 2024

Description

Fixes #3847
The Makie 0.21 version doesn't propagate convert_arguments arguments to convert_arguments(trait) directly, making some common conversions impossible (#3847 for example). Turns out, the fix (this PR) is pretty simple.

Type of change

Delete options that do not apply:

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

Checklist

  • Added unit tests for new algorithms, conversion methods, etc.

@SimonDanisch
Copy link
Member

The new expand_dimensions was introduced for a reason, so this should not be the correct fix...
I'm surprised that this passes tests though, so I guess I need to investigate this a bit.
In 0.21 you will need to start overloading expand_dimensions for your example. This should get documented a bit better!

@aplavin
Copy link
Contributor Author

aplavin commented May 13, 2024

This PR doesn't remove expand_dimensions, it still gets called one step downstream at

function convert_arguments(CT::ConversionTrait, args...)
expanded = expand_dimensions(CT, args...)
if !isnothing(expanded)
return convert_arguments(CT, expanded...)
end
return args
end
.
That's why the tests keep working.

@SimonDanisch
Copy link
Member

Ok thank you! I still think this was covering some edge case and that for your overload to work properly you will need to switch to overloading expand_dimensions. But I may be mistaken, those last changes where done a bit in a rush.
I will need to take a look at it tomorrow.

@aplavin
Copy link
Contributor Author

aplavin commented May 13, 2024

If arguments are not propagated directly from convert_arguments(plottype) to convert_arguments(trait) (as it currently is in 0.21), then the easiest way to actually overload convert_arguments for all point-based plots is to define a bunch of convert_arguments(::Type{Scatter}), convert_arguments(::Type{Line}), ... manually. This is definitely more approachable than being required to plug into yet another mechanism, expand_dimensions.

Defining all those methods manually is feasible, of course, but would be unfortunate to lose the simplicity and convenience of overloading convert_arguments(::PointBased) that works in 0.20.

@SimonDanisch
Copy link
Member

We will need to think more about how to streamline x,y,z arguments in Makie and how to decouple it from convert_arguments.
Since i couldn't find any corner case that stopped working with this, we might as well merge it for now!

@SimonDanisch SimonDanisch merged commit 0516496 into MakieOrg:master May 14, 2024
17 of 18 checks passed
@SimonDanisch
Copy link
Member

Thank you!

@aplavin aplavin deleted the patch-1 branch May 27, 2024 22:38
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.

conversion traits not taken into account in 0.21
2 participants