Conversation
ctrueden
changed the title
ctrueden
approved these changes
Nov 1, 2022
Member
ctrueden
left a comment
ctrueden
left a comment
There was a problem hiding this comment.
Thanks @elevans. I reviewed this PR thoroughly, and force-pushed an improved version of the logic. There were a few things that needed to be addressed:
- I changed the kwargs name from
kwargstohints, since the intent is for these key/value pairs to provide hints on specifics of the conversion operation. - I bumped the minor version (1.7.1 → 1.8.0) because this is new API: it modifies existing functions to now have kwargs support, which is a backwards-compatible API change.
- There were no tests, so I remedied that. I added new Python-to-Java converters for
int-to-Byteandint-to-Shortin case the hinttype='byte'ortype='short'respectively is given. While at it, I also madetype='long'work to convert tolongwhen values are withinintboundaries (although I didn't test this latter feature yet 😐). - In the course of writing tests, I discovered a bug: the hints mechanism was not propagating hints to the predicate functions, only to the converter functions. So I fixed that as well.
- In the course of doing all this, it became convenient to introduce a new
jinstancefunction for testing whether an object is an instance of a particular Java class. This was useful for testing immediately, but will also be more important later as the JEP work continues, sinceisinstanceunfortunately does not behave properly with JEP as of this writing. - Finally, I did a couple of other miscellaneous cleanups.
Now that all of that is addressed, I think this PR is looking good to merge!
Member
|
There is one problem though: the CI is currently broken on the master branch. We should fix that first, and then rebase this PR one more time on top, to be sure all checks continue to pass. |
ctrueden
force-pushed
the
to-xarray
branch
2 times, most recently
from
f2f6291 to
7f54af5
Compare
Certainly we require at least OpenJDK 8, although on conda-forge, 8.0.112 is the oldest anyway, so this rule has no effect in practice. For the developer environment, for now we restrict to OpenJDK 11 maximum, not OpenJDK 17, because we want CI to use OpenJDK 11 (the scijava-tables library has a bug with OpenJDK 17 as of this writing). In the long term, restricting OpenJDK in this way is the wrong thing to do, but it's tolerable for now to get tests passing.
There were two conflicting assumptions in the codebase: 1. If the library is not found, a falsy value is returned; and 2. If the library is not found, an exception is raised. When initializing converters, it's looking for the falsy value, i.e. just a simple test of whether the dependency is available. But when importing the dependency in a function that actually uses it, an exception is preferred, to ensure the import never ends up as None. This commit fulfills both scenarios by adding a boolean required flag to the respective import functions. If a gentle check for availability is preferred, one can pass required=False to obtain that behavior now.
This is more consistent with the other imports from typing. We didn't do it before because previously, List was a property for the jimported java.util.List class. But now all imported Java classes are inside the JavaClasses struct, so we're OK.
* Fix float and double converter predicates. * Add tests for floating point infinity and NaN. * Use clearer and more consistent variable names. * Use jinstance function to test converted types. * Assert converted types and values more consistently. * Remove the convoluted stringified asserts.
Hints are freeform key/value pairs that converters may use if desired to affect whether and how particular objects are converted between types. Co-authored-by: Curtis Rueden <ctrueden@wisc.edu>
And while we're at it, stabilize the Converter API. Rather than calling the predicate and converter functions directly, we now introduce stable supports and convert methods that always accept hints kwargs, regardless of whether the linked predicate and/or converter functions do.
And test them, to improve confidence that conversion hints work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intro
This PR adds
**kwargstoscyjava's converter mechanism. This change is necessary to support theto_xarray()changes in the pipe forpyimagej(see imagej/pyimagej#231) for more details.Features
**kwarg**support -- enablesij.py.to_java(data, dim_order=["x", "y", "z"])inspect.signaturedoesn't work on Java objects. Here we just drop thekwargs. I think this is better than trying to add signature detection for Java methods.