use concrete type instead of abstract Point2 #23
Conversation
There was a problem hiding this comment.
I would not create a new file specific to issues, it might be better to append it to the reference tests?
There was a problem hiding this comment.
I wanted to have only tests that compare produced plots/images against reference images in the referencetests.jl file to keep the different types of tests separate, but I would of course be fine to move them if you insist :)
|
Struggling a little with the image sizes still. It seems to be dependent on what display one is working. |
test/referencetests.jl
Outdated
| #Somehow fig seems to get converted in AbstractArray{<:Colorant} of different sizes, depending on | ||
| #what display is used; the function below is a workaround through file saving/loading. Not | ||
| #elegant, but it works. | ||
| function save_load_remove(fig) |
There was a problem hiding this comment.
I asked on Slack and one can use colorbuffer(fig) to directly get a Matrix of Colorant.
There was a problem hiding this comment.
Yes, but at least when I am testing it locally, I get the wrong size of matrix out. Doing:
fig = Figure(size = (900, 600), px_per_unit = 2)
arr = colorbuffer(fig)
I would have expected an array of 1800 x 1200 pixels, but I am getting a 900×1350 PermutedDimsArray(view(::Array{RGB{N0f8},2}, :, 900:-1:1), (2, 1)) with eltype ColorTypes.RGB{FixedPointNumbers.N0f8} returned to me.
Furthermore, what I am getting back is dependent on what display/monitor I am having the figure open on! I have two monitors: monitor 1 gives the above. When I move the same figure and run arr = colorbuffer(fig) again, I am getting a 604 x 907 matrix. Monitor 1 is landscape, monitor 2 is portrait.
That kind of made me move in the direction of a workaround...
Is there a direct and robust way of doing this correctly?
There was a problem hiding this comment.
Hmm let me ask again on Slack if there is a standard way to do this.
There was a problem hiding this comment.
But anyway that's off topic from this MR. Let's just get this merged and we can apply an eventual better solution in a separate MR.
There was a problem hiding this comment.
Solution seems to be colorbuffer(fig; px_per_unit=2) ?
There was a problem hiding this comment.
Added a new commit; needed some more tweaking, but I think this version is cleaner than before.
|
Hi guys, sorry for being permanently afk on this repo. This PR looks good to me, I will merge it and release. Thanks for helping out here! |
Should address #22
Key change is that a concrete type
Point2fis used for the datapoints in scatter (and axis) instead of the abstract typePoint2. This essentially led to anAnytype ending up somewhere down in the Makie stack where it didn't belong.