Make expand_dimensions definitions non ambiguous #795

asinghvi17 · 2024-09-02T18:30:51Z

Update README.md formatting

ext/DimensionalDataMakie.jl

since that's what we'd do anyway

asinghvi17 · 2024-09-03T23:33:21Z

I am now routing all convert_arguments through the conversion trait for the overridden functions. Since we don't dispatch on raw plot types for these in convert_arguments anyway, this seems reasonable to me.

This also gets rid of plots not converting correctly.

The currently outstanding issue with this approach, is that updating plots created in this way isn't possible with traditional updating syntax (plt[1] = new_dimarray). You have to pass in an Observable or do the conversion and unwrap it yourself. Both of which are bad ideas. That's next on the list but would involve cleaning up the whole recipe system anyway.

This also fixes the convert_args for vertexgrid which was using the wrong array for lookup, causing y-flipped contourf for example.

asinghvi17 · 2024-09-05T02:41:39Z

Ah, categorical lookups have to be handled like regular ones.

ext/DimensionalDataMakie.jl

rafaqz · 2024-09-05T06:55:27Z

ext/DimensionalDataMakie.jl

+function _lookup_to_vertex_vector(l)
+    if isintervals(l)
+        bs = intervalbounds(l)
+        return @. first(bs) + (last(bs) - first(bs)) / 2


Isn't this just parent(shiftlocus(Center, l)) ?

Then we don't need to check Points/Intervals either.

Is it? If so that would be great. Does shiftlocus also handle converting point sampling to interval sampling?

No you have to do that separatly: shiftlocus(Start(), set(lookup, Intervals())). Points will always turn into Intervals(Center()) by default.

(mostly because shifting Points() to Center() could still give Points, so its kind of ambiguous)

Its good to always use the methods like shiftlocus for things like this so we can centralise handling of the edge cases.

ok people. I just tested this one in combination with rafaqz/Rasters.jl#723, and things seems to work really well. So, I would like to merge this one, so that we can also fix Rasters#main.

lazarusA

thanks a lot @asinghvi17 for bringing this up to the finish line. Great work!

Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>

lazarusA · 2024-09-09T09:02:50Z

ext/DimensionalDataMakie.jl

+        The lookup was:
+        $l
+
+        You can solve this by resampling your raster, or by using a more permissive plot type like `heatmap`, `surface`, `contour`, or `contourf`. 


@rafaqz I just hit this edge case for raster's volume-like cases. what would be the way to get a regular-spaced raster? a better suggestion is needed here. Maybe one-liner to resample a given raster A?

Ah yeah a one liner would be good to add there. I should also add an option to customize the error message so this can be used in volume plots...

With image or heatmap? heatmap should work Maybe we can just warn for image and plot anyway?

the issue I found was for something with X, Y, Ti, creating a volume, where the axis for Ti is usually irregular, hence failing afterwards to plot, I just want say, treat this as a regular thing as well, it makes sense for daily data, but I'm sure there should be others where we should not do that.

Aha ok! Yeah I guess it's a bit restrictive to error all the time.

asinghvi17 · 2024-09-09T16:26:15Z

TODOs on my end are:

Special-case axis and figure attributes so they DON'T get converted to Observables.
Better and more configurable error messages when checking regular lookups
Use shiftlocus and set when converting lookups to vectors.

ext/DimensionalDataMakie.jl

They're my special snowflakes ♡ヮ♡

lazarusA · 2024-09-11T14:34:41Z

something went wrong with the latest axis, args removal.

asinghvi17 · 2024-09-11T14:48:00Z

Yeah it's just passing a bunch of axis kwargs to LScene when it shouldn't. Will look at that.

asinghvi17 · 2024-09-11T23:14:36Z

Things look good enough to me. @rafaqz feel free to merge whenever.

rafaqz · 2024-09-12T05:04:45Z

ext/DimensionalDataMakie.jl

            x=nothing, y=nothing, colorbarkw=(;), attributes...
        )
            replacements = _keywords2dimpairs(x, y)
            _, _, args, _ = _surface2(A, $f2, attributes, replacements)
-            # No ColourBar in the ! in-place versions
-            return Makie.$f2!(axis, args...; attributes...)
+            # No Colorbar in the ! in-place versions


Hey I'm Australian ;)

rafaqz · 2024-09-12T06:10:50Z

ext/DimensionalDataMakie.jl

+        The lookup was:
+        $l
+
+        You can solve this by resampling your raster, or by using a more permissive plot type like `heatmap`, `surface`, `contour`, or `contourf`. 


With image or heatmap? heatmap should work Maybe we can just warn for image and plot anyway?

It semi works but the dimensions are still wrong, probably because of Makie converts..

rafaqz · 2024-09-14T19:50:54Z

Are the spy fixes good for merging now too?

asinghvi17 · 2024-09-14T21:22:35Z

Yes, I'd say it's better than nothing. We can only fix the remaining issues on the Makie end.

asinghvi17 and others added 3 commits September 2, 2024 11:30

Make expand_dimensions definitions non ambiguous

0462a4c

Update README.md formatting

1b8b604

Merge pull request #796 from rafaqz/as/update_readme

29cc06f

Update README.md formatting

lazarusA reviewed Sep 3, 2024

View reviewed changes

ext/DimensionalDataMakie.jl Show resolved Hide resolved

asinghvi17 added 4 commits September 3, 2024 10:53

Fix vertexgrid conversions in DimensionalData

980afe6

Fix all issues by using conversion trait only in 2d plots

2550555

since that's what we'd do anyway

Use a more intelligent method to check the axis type

c30b51b

Get tests going

3c46d2a

asinghvi17 added 3 commits September 4, 2024 09:39

Add comments + regular sampling check function

994ed92

Drop Makie v0.19 compat, bump patch version

653a90d

Drop Makie v0.19 compat, add checks for regular sampling

1855277

This also fixes the convert_args for vertexgrid which was using the wrong array for lookup, causing y-flipped contourf for example.

Regular and categorical sampling are both acceptable

9f17116

asinghvi17 requested a review from lazarusA September 6, 2024 05:51

rafaqz reviewed Sep 6, 2024

View reviewed changes

lazarusA approved these changes Sep 9, 2024

View reviewed changes

lazarusA mentioned this pull request Sep 9, 2024

Continue patch rafaqz/Rasters.jl#723

Open

Apply suggestions from code review

ededfb8

Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>

lazarusA reviewed Sep 9, 2024

View reviewed changes

rafaqz reviewed Sep 9, 2024

View reviewed changes

ext/DimensionalDataMakie.jl Outdated Show resolved Hide resolved

ext/DimensionalDataMakie.jl Outdated Show resolved Hide resolved

asinghvi17 added 4 commits September 9, 2024 15:03

Use DD.shiftlocus for intervals to points

32c8840

Treat axis and figure keyword arguments differently

b2913ba

They're my special snowflakes ♡ヮ♡

Handle axis + figure attributes better

f73b55e

remove axis kwargs from where they shouldn't be

189456d

fix wrong attr passing when axis type is known

60715cb

rafaqz approved these changes Sep 12, 2024

View reviewed changes

rafaqz mentioned this pull request Sep 12, 2024

spy(dd) not working because of ambiguity #800

Open

asinghvi17 added 4 commits September 12, 2024 07:39

Warn only on irregular lookup

eeee224

more information about why this is bad

d5126f9

Define a fallback for NoConversion traits

7f446cb

Specific convert for spy to compute intervals

3c6f2e5

It semi works but the dimensions are still wrong, probably because of Makie converts..

rafaqz merged commit d857504 into main Sep 14, 2024
8 checks passed

rafaqz deleted the asinghvi17-patch-3 branch September 14, 2024 23:49

This was referenced Sep 19, 2024

image should use extrema of dimensions and not the full lookup #779

Closed

Plotting deprecations in tests #732

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make expand_dimensions definitions non ambiguous #795

Make expand_dimensions definitions non ambiguous #795

asinghvi17 commented Sep 2, 2024 •

edited

Loading

asinghvi17 commented Sep 3, 2024

asinghvi17 commented Sep 5, 2024

rafaqz Sep 5, 2024

asinghvi17 Sep 7, 2024

rafaqz Sep 7, 2024 •

edited

Loading

rafaqz Sep 7, 2024

lazarusA Sep 9, 2024

lazarusA left a comment

lazarusA Sep 9, 2024

asinghvi17 Sep 9, 2024

rafaqz Sep 12, 2024 •

edited

Loading

lazarusA Sep 12, 2024

asinghvi17 Sep 12, 2024

asinghvi17 commented Sep 9, 2024 •

edited

Loading

lazarusA commented Sep 11, 2024

asinghvi17 commented Sep 11, 2024 •

edited

Loading

asinghvi17 commented Sep 11, 2024 •

edited

Loading

rafaqz Sep 12, 2024

rafaqz Sep 12, 2024 •

edited

Loading

rafaqz commented Sep 14, 2024

asinghvi17 commented Sep 14, 2024

Make expand_dimensions definitions non ambiguous #795

Make expand_dimensions definitions non ambiguous #795

Conversation

asinghvi17 commented Sep 2, 2024 • edited Loading

asinghvi17 commented Sep 3, 2024

asinghvi17 commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaqz Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazarusA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaqz Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asinghvi17 commented Sep 9, 2024 • edited Loading

lazarusA commented Sep 11, 2024

asinghvi17 commented Sep 11, 2024 • edited Loading

asinghvi17 commented Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

rafaqz Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

rafaqz commented Sep 14, 2024

asinghvi17 commented Sep 14, 2024

asinghvi17 commented Sep 2, 2024 •

edited

Loading

rafaqz Sep 7, 2024 •

edited

Loading

rafaqz Sep 12, 2024 •

edited

Loading

asinghvi17 commented Sep 9, 2024 •

edited

Loading

asinghvi17 commented Sep 11, 2024 •

edited

Loading

asinghvi17 commented Sep 11, 2024 •

edited

Loading

rafaqz Sep 12, 2024 •

edited

Loading