-
Notifications
You must be signed in to change notification settings - Fork 51
Breaking: Extent passthrough for multidimensional lookups #991
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
base: breaking
Are you sure you want to change the base?
Conversation
|
I know this is mostly an internal thing - but I don't know if I find |
|
Its for these mutidimensional dims you can index with the internal dimensions, like X/Y for Geometry |
|
I prefer |
|
That could also be e.g. |
| Base.CartesianIndices(dims::DimTuple) = CartesianIndices(map(d -> axes(d, 1), dims)) | ||
|
|
||
| # Extents.jl | ||
| #= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that we still had the old version to refer back to, mainly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can just remove it, but we probably need an explanation for the new complexity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, now that it works I'll do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm waffling on whether we should define hasmultipledimensions for dimensions...
|
|
||
| multidims = otherdims(ds, regulardims) | ||
| multidim_raw_bounds = bounds.(multidims) # we trust that bounds will give us a tuple of bounds one for each enclosed dimension | ||
| multidim_raw_bounds = map(bounds, multidims) # we trust that bounds will give us a tuple of bounds one for each enclosed dimension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be made into a contract, perhaps in the docs of hasmultipledimensions and MultiDimensionalLookup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm waffling on whether we should define hasmultipledimensions for dimensions...
|
Any updates on this? I want to merge the breaking branch soon... was this really breaking? If so would be good to combine |
|
Just need to add docs really But feature wise it is complete |
|
Seems like it needs a few tests too |
|
Yeah that too! Might get claude to do that. |
…991) - Add comprehensive tests for hasmultipledimensions trait in merged.jl - Test that regular lookups return false for hasmultipledimensions - Test that MergedLookup returns true for hasmultipledimensions - Test extent passthrough for merged dimensions - Test mixed regular and merged dimensions - Test that operations preserve the hasmultipledimensions trait - Add fallback methods to handle edge cases: - hasmultipledimensions(::Any) = false for non-Lookup types - bounds(x::AbstractArray) for raw arrays - Import Extents in merged.jl test file - Fix bounds ambiguity with explicit module qualification
hasmultipledimensions(name controversial)