feat: define conversion rules #59
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
==========================================
+ Coverage 72.03% 74.06% +2.02%
==========================================
Files 4 4
Lines 1613 1666 +53
==========================================
+ Hits 1162 1234 +72
+ Misses 451 432 -19 ☔ View full report in Codecov by Sentry. |
|
|
||
| [extras] | ||
| PythonCall = "6099a3de-0909-46bc-b1f4-468b9a2dfc0d" |
There was a problem hiding this comment.
@Moelf - it could be due to my own setup, but I did not manage to get this PythonCall dependency when running from Python REPL. This is an attempt to fix it.
There was a problem hiding this comment.
before it was set up in a way such that installing AwkwardArray.jl does NOT imply needing a Python. If I were to do this, I would just make sure from the python side, it calls something to install PythonCall.
The way this PR implements it, you cannot use this package as a pure-Julia package, which may or may not be what you want.
There was a problem hiding this comment.
That's what we discussed today. Although it would be better (more opportunities) for this to be usable as a pure-Julia package, @ianna ran into some problems attempting to do so, and I think its primary value would be for users of both Python and Julia. (In Julia, there are already packages for some of these use-cases, such as ArrayOfArrays and StructArrays.)
So the decision between optionally depending on Python and strictly depending on Python is the result of a cost-benefit analysis. The benefit is small but positive; how big is the cost?
There was a problem hiding this comment.
the way this PR does it sounds reasonable to me.
| function pyconvert_rule_awkward_array_primitive(::Type{AwkwardArray.PrimitiveArray}, x::Py) | ||
| array = AwkwardArray.convert(x) | ||
| return PythonCall.pyconvert_return(array) | ||
| end |
There was a problem hiding this comment.
These rule functions should return pyconvert_unconverted() if the conversion was not possible.
There was a problem hiding this comment.
Is that distinct from encountering an Exception and raising it?
For example, in Python, some overloading functions let you return NotImplemented to say, "I'm not handling it; let someone else handle it," but raise XYZ is different.
For Awkward Array conversion, there isn't any Python Awkward Array that would return pyconvert_unconverted() rather than a Julia Awkward Array or vice-versa. But it could encounter an exception in the attempt (e.g. out of memory or recognizing that the array doesn't satisfy a validity rule if the Python Awkward Array was hacked together with __new__ or something).
There was a problem hiding this comment.
These rule functions
shouldmust returnpyconvert_unconverted()if the conversion was not possible.
There was a problem hiding this comment.
I made some comments below—I should have done it as part of this review—but other than that, this looks good!
We discussed at our meeting that making PythonCall a required dependency is okay. The main benefit of this library is for users of both Python and Julia, since Julia users already have ArrayOfArrays and such. I think the use of this library for Julia only is limited (though not nonexistent).
So I think this is good to merge as-is. There are some open questions (open comment boxes) that @Moelf could help us on, but they're longer-term than this PR.
restructure to remove weak dependencies: it seems that Python REPL does not allow to see a PythonCall requirement