Open visr opened 2 years ago
I think its more generic than this... basically anything that defines isless
could be acceptable for extent bounds. Like Char
will still work fine currently. I'm not sure that it shouldn't?
So per dimension we would just have Tuple{<:Any,<:Any}
, lacking any kind of abstract type <: Sortable
.
Even if not all sortables worked, we would need at least Number
so Unitful.jl works.
Also need to think about the order check, because that will be hard to go back on.
basically anything that defines isless could be acceptable for extent bounds
Yeah whilst preparing this I realized there is no need to restrict it to real, and that it can be useful for other types as well. It'd be good to add some tests for these other types as well.
If you prefer I can reduce this to only check for length 2 tuples and leave the sorting for later. You're right that it'd be hard to go back on, though I cannot really come up with a reason not to do that other than the antimeridian handling. An extent is not really a directional thing, so therefore a guaranteed order can be useful.
Sorry I totally forgot about this.
I think you're right, the order should be forced. I'm treating it like it is in all my code that uses an Extent
already.
So yes - checking for sorted length 2 tuple is a good idea. We can just swap those Real
s for Any
s and merge.
One minor concern is that we will then be checking that things are sortable by sorting them, which will have unfortunately vague errors if they cant be sorted. But there's not much we can do that has reasonable performance.
Edit: I just realized this will break DimensionalData.jl for unordered dimensions, but that's because it doesn't do anything sensible currently. We may need a way to mark that there is a dimension that exists but doesn't have a known or sensible extent, say by using D=(nothing,nothing)
or just D=nothing
. Then we can skip the minmax
for nothing
.
:exclamation: No coverage uploaded for pull request base (
main@bc54a82
). Click here to learn what that means. The diff coverage isn/a
.
@@ Coverage Diff @@
## main #6 +/- ##
=======================================
Coverage ? 82.75%
=======================================
Files ? 1
Lines ? 87
Branches ? 0
=======================================
Hits ? 72
Misses ? 15
Partials ? 0
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
For unordered collections, is it better to use nothing
, or should we just not order them? Something like:
applicable(isless, a, b) ? minmax(a, b) : (a, b)
applicable
is slow tho right?
I normally try to avoid using reflection tools in low level code.
I guess, yeah. Wonder what is best then. Also considering Vectors with missing or NaN. We could also just not sort in the inner constructor, but document what it should be in those cases.
Fixes #4.
This now enforces 3 things:
Do we want to enforce all these 3? I'm not sure about Real, perhaps anything that has order is fine. E.g. do we want this to work with say a 2D netCDF matrix that has Real Z and String Station dimensions? Or for instance with Unitful.jl, which are Number, not Real?
And does it make sense to sort on construction, or would it be preferred to error if it is not sorted?