-
Notifications
You must be signed in to change notification settings - Fork 49
Breaking: Materialize DimArray
or DimStack
From a Table
#739
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
Conversation
Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>
…a.jl into materialize
There's still a few questions that need resolving:
|
I think it's best to test that the geometry column's elements have Going forward to get the geometry column it's probably best to have |
DimensionalData.jl does not depend on GeoInterface |
Being able to interact with geometries in a generic fashion would help us interop with other packages. I also see that |
It's not about deps or timing, it's about clean feature scope. The promise here is that Rasters (and YAX) have all the geo deps and features, so non-geo people don't have to worry about them. Some of the biggest contributors here use DD for unrelated fields. I would ignore point columns here entirely and instead write the code so it's easy for Rasters to handle them. Taking the underscores off a few functions and documenting them as an real interface will help that. |
I've updated the docstrings for both the |
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 is good, but a few minor changes are needed for it to be correct. Constructed selectors should be allowed so At can have atol
. We can just construct selector types at the outer level (they're just filled with nothing
.
Then there is a bit more dispatch needed to make the fast paths correct for At/Near/Contains with their standard behaviour.
Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>
Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>
…a.jl into materialize
Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>
Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>
Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #739 +/- ##
============================================
- Coverage 84.62% 84.18% -0.45%
============================================
Files 53 54 +1
Lines 5307 5456 +149
============================================
+ Hits 4491 4593 +102
- Misses 816 863 +47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Okay docs fails because of this:
But anyway I think all of these functions should be internal. Should I just remove the docstrings - or make them into comments or something? (Never seen this before so I'm not 100% about what this is testing) |
Yeah we force docstrings to be in the docs. I think these should just be comments not docstrings |
All right let's see if this passes now. I think before merging we should add some documentation, though. Another thing is that this will only work with exactly I've tried to solve this now by adding a type argument to |
Yes perfect that's why I removed the underscores, those a_from_b methods can become dev api |
@tiemvanderdeure this looks good to merge ? |
Yeah I think it is! I don't have more, if you've reviewed it and found nothing then do merge :) |
I think this got merged into main and released. Was that on purpose? It is very slightly breaking. |
It got merged into breaking! which I do want to merge now but it will get a breaking bump |
Ah I got confused because it is listed in the tag https://github.com/rafaqz/DimensionalData.jl/releases/tag/v0.29.21 Didn't know that it also listed PRs into other branches than main |
Oh I didn't know that either.. thats actually bad |
Description:
resolves #335
This PR aims to let users construct either a
DimStack
orDimArray
from a table with one or more coordinate columns.Unlike the existing contructor, rows may be out of order or even missing altogether.
Performance:
The algorithm is O(n), requiring two forward passes for each dimension to determine the correct order of rows.
1000x1000: 0.005 Seconds
2000x2000: 0.025 Seconds
4000x4000: 0.108 Seconds
8000x8000: 0.376 Seconds
Example:
Next Steps:
missing
by default).:geometry
column.