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

convert_single_argument isn't sufficient for vector types #3509

Closed
3 tasks done
jakubkaczor opened this issue Dec 24, 2023 · 2 comments
Closed
3 tasks done

convert_single_argument isn't sufficient for vector types #3509

jakubkaczor opened this issue Dec 24, 2023 · 2 comments
Labels

Comments

@jakubkaczor
Copy link

jakubkaczor commented Dec 24, 2023

  • are you running newest version (version from docs) ? Yes.
  • can you reproduce the bug with a fresh environment ? (]activate --temp; add Makie). Yes.
  • What platform + GPU are you on?
    OS: Arch Linux x86_64
    CPU: AMD Ryzen 5 5600U (12) @ 4,29 GHz
    GPU: AMD Radeon Vega Series / Radeon Vega Mobile Series

For any type T, I expect implementing convert_single_argument(::T)::S, where S is a type that can be handled by Makie, to be sufficient for T being handled by Makie, but there is a type U and T = Vector{U}, so this is not the case. Here is a minimal example of such type U.

using GLMakie
import GeometryBasics as Geom

# Example of the U type
struct UnitSquare
  origin::Point2
end

Makie.convert_single_argument(ss::Vector{UnitSquare}) = map(ss) do s
    Geom.Rect(s.origin..., 1, 1)
end

square = UnitSquare(Point2(0, 0))
data = [square]
after_conversion = Makie.convert_single_argument(data)
expected_conversion = [Geom.Rect(0, 0, 1, 1)]

# Although the types are the same
@assert typeof(after_conversion) == typeof(expected_conversion)
# This *doesn't* work as expected and poly([square]) fails completely
poly([square])
# although the following works
poly(expected_conversion)

The poly([square]) results in

ERROR: Makie.convert_arguments for the plot type Scatter and its conversion trait PointBased() was unsuccessful.

The signature that could not be converted was:
::Vector{GeometryBasics.HyperRectangle{2, Int64}}

When I implement convert_single_argument for UnitSquare instead of a vector of UnitSquare, the following works as expected.

Makie.convert_single_argument(s::UnitSquare) = Geom.Rect(s.origin..., 1, 1)
poly(square)
@SimonDanisch
Copy link
Member

Hm this could be a because the poly recipe does quite a bit of it's own conversion:
https://github.com/MakieOrg/Makie.jl/blob/master/src/basic_recipes/poly.jl#L121

@jakubkaczor
Copy link
Author

Oh, I see. Is this something that should be reflected in the documentation, or in the code? I think the latter, but correct me if I am wrong.

SimonDanisch added a commit that referenced this issue Apr 9, 2024
SimonDanisch added a commit that referenced this issue Apr 30, 2024
* take over most of the work from #1347

* add typed argument conversion (#3565)

* add typed argument conversion

* fix volume

* add function to get available conversions

* make conversion apply more narrowly

* more cleanly separate recursion in convert_arguments

* clean up

* allow to get axis before creating a plot

* clean up

* fix tests

* bring back dim converts (axis_convert)

* update tests

* fix tests and work around conversion problems

* fix WGLMakie

* fix errors

* clean up conversion pipeline

* fix tests

* add changelog entry

* disable project run

* improve performance slightly

* might as well use array

* tmp

* wip

* implement axis convert recursion

* fix tests

* fix datashader

* fix datashader

* move unitful integration

* fix performance regression!?

* fix merge & new date time improvements

* fix scaling test

* remove test false

* clean up

* converts shouldnt be here

* move axis converts to scene

* further refactor [skip ci]

* finish refactor for AxisConversion type

* allow limit setting and ticks

* make tests less noisy

* cleanup

* clean up and fix unitful/date conversion

* make sure all tests work correctly

* remove rand

* rename, clean up and make axis spec work

* clean up and test new conversion pipeline

* undo feature deletion, don't reintroduce Rect2f

* be explicit about Volume Interval types

* minor docstring cleanup

* try to clarify new conversion docstrings

* remove convert_arguments_typed in favor of types_for_plot_arguments

* fix remaining bugs for conversion simplification

* fix ticks not updating

* fix specapi

* fix qqnorm

* clean up types_for_plot_arguments

* fix tuple conversion

* try to fix compile time regression

* try to fix compile time regression

* clean up and introduce expand_dimensions

* fix #3655 and clean up convert_arguments + add tests

* fix #3509 and add tests for

* clean up observables and more docs

* final rename

* fix docs

* cleanup

* small clean up

* small doc improvements

* improve docs

* fix docs

* try relative link

* try without .md

* take out link

* try fix

---------

Co-authored-by: ffreyer <frederic481994@hotmail.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants