-
Notifications
You must be signed in to change notification settings - Fork 47
Add support for zero-copy conversions from an xarray to a DimArray #972
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #972 +/- ##
==========================================
- Coverage 83.44% 83.44% -0.01%
==========================================
Files 54 54
Lines 5154 5152 -2
==========================================
- Hits 4301 4299 -2
Misses 853 853 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I'm happy for this not to be breaking, I would assume the output is just some Maybe make it clear in the docs that the behavior is generally guaranteed by semver but the type is not, so this is clear in future. (It can also be lumped with the breaking changes, but might wait a month in that case) |
Roger that, updated the docs in 9448a41. |
If PythonCall installed a version of OpenSSL incompatible with what Julia is built with and it loads the newer version first, that will conflict with any other Julia package (e.g Plots.jl) that tries to load OpenSSL afterwards. Here we hack around that by just loading OpenSSL.jl first so that Python uses that.
Blegh, CI was hitting some awful OpenSSL incompatibility: I attempted to fix it in 208c3a2. |
Yeah, having the python deps in out tests is a little problematic... we have S tier rock-solid package management from Pkg for everything else, but we lose that reliability by inserting a Python package manager into the workflow. Most devs here including me don't know how to fix problems with python deps, so I am hoping this can kind of be your responsibility! |
Yes I can do that 🫡 Luckily it looks like that hack worked. |
Let's port that fix to Rasters too? Looks like the same issue there. |
The PythonCall fix? Does Rasters integrate with PythonCall? |
In the process I also refactored the code to rely on PythonCall more, which simplified it quite a lot. One thorny issue is that this could be considered a breaking change since the parent array type changed from
Array
toPyArray
, which is not aDenseArray
(mentioned in the new docs). But I feel that the parent array type is a pretty internal thing to change, and since in the worst case it won't break any existing code but just make it slower, maybe we can still put it in a patch release?