Rewrite to_java and to_python as a set of converter objects

[gselzer]

This PR seeks to do away with the confusing case logic of the to_java and to_python functions. Towards this effort, we introduce Converter:

class Converter(NamedTuple):
    predicate: Callable[[Any], bool]
    converter: Callable[[Any], Any]
    priority: float = Priority.NORMAL

We then consolidate a number of these Converters into two maps: one holds all Converters going from Java to Python, and the other holds all Converters going from Python to Java.

This reduces to_java to:

def to_java(obj: Any) -> Any:
    """
    docstring omitted
    """
    start_jvm()
    return _convert(obj, java_converters)

where

def _convert(obj: Any, converters: typing.List[Converter]) -> Any:
    suitable_converters = filter(lambda c: c.predicate(obj), converters)
    prioritized = max(suitable_converters, key = lambda c: c.priority)
    return prioritized.converter(obj)

to_python is similarly reduced to a call to _convert with py_converters

The Converters lists are exposed as global variables; this is intended to allow users of ScyJava (namely PyImageJ, and likely napari-imagej) to add Converters. By using high priority, this should allow those projects to also overwrite the Converters of parent projects.

As of this writing, all commits build with passing tests.

[hinerm]

This looks well-designed and easy to use.

I still think it would be nice to explore using something like SortedList for the converter data structures, to allow short-circuiting the search but it's not essential and wouldn't affect the API if done later.

I also have two thoughts related to typing...

• Converter could be rewritten to use TypeVar + Generic so that mypy will catch the creation of Converters with incompatible predicate + callable pairs
• I can envision frustration when a user adds a Converter that doesn't take effect e.g. because there is a higher-priority converter that operates on the same type or a supertype. It might be nice, when adding a Converter, to see if there are other converters that operate on the same (super)type. I looked at inspect a bit but think this would only be reliable if there was an instance of the generic type was taken as a param. Anyway this is solving a problem that hasn't been raised and probably never will be, but wanted to think about it in case there's a simpler solution.

[gselzer]

Thanks for looking it over @hinerm!

I still think it would be nice to explore using something like SortedList for the converter data structures, to allow short-circuiting the search but it's not essential and wouldn't affect the API if done later.

I can't say I'm against this, but we would be adding another dependency. If that is of no consequence, then I can do it.

• Converter could be rewritten to use TypeVar + Generic so that mypy will catch the creation of Converters with incompatible predicate + callable pairs

I don't think that is allowed. See this SO post. We could make Converter a class though, which would allow us to use Generic...

• I can envision frustration when a user adds a Converter that doesn't take effect e.g. because there is a higher-priority converter that operates on the same type or a supertype. It might be nice, when adding a Converter, to see if there are other converters that operate on the same (super)type. I looked at inspect a bit but think this would only be reliable if there was an instance of the generic type was taken as a param. Anyway this is solving a problem that hasn't been raised and probably never will be, but wanted to think about it in case there's a simpler solution.

We could add a function converters_satisfying that returns all Converters with predicates valid for a given Object. I only see that function being used for debugging purposes, though...

[hinerm]

@gselzer

I don't think that is allowed. See this SO post. We could make Converter a class though, which would allow us to use Generic...

OK this is my python lack-of-knowledge problem but what's the difference between the NamedTuple constructor you build and manually defining it?

from typing import TypeVar, Generic, Any

T = TypeVar('T')

class Converter(Generic[T]):
    def __init__(self, predicate: Callable[[T], bool], converter: Callable[[T], Any], priority: float = Priority.NORMAL) -> None:
        self.predicate = predicate
        self.converter = converter
        self.priority: = priority

[gselzer]

@hinerm at this point, nothing 😆

In earlier stages of my design I was not planning to promote the Converter class for external usage. I figured people might instead pass something that would be easily convertable to a Converter tuple. Now that it is promoted, I think we should do what you suggest.

[ctrueden]

On a different note: the use of the Java-side Priority class for the priority constants makes me nervous. In @hinerm's blurb above, the priority attribute is assigned a default value of Priority.NORMAL, but that assumes the JVM is already started. I saw that @gselzer had some trickery to delay registering these converters until after the JVM starts up, but that feels dangerous to me. Is there some reason we don't just replicate the priority constants in Python? They're very simple, and will never change in the future. It is also not good for scyjava to have any dependency on scijava-common!

Edit: N/m, I see that in fact, there is a Python-side priority class. So, good! 👍

[gselzer]

Now that it is promoted, I think we should do what you suggest.

Okay, @hinerm, now that I tried this, I am less in favor of it 😅

To start, we actually don't want Converter.predicate to be a Callable[[T], bool], we want it to be a Callable[[Any], bool]; this function should be analogous to Converter.canConvert()

With this in mind, we'd have:

from typing import TypeVar, Generic, Any

T = TypeVar('T')

class Converter(Generic[T]):
    def __init__(self, predicate: Callable[[Any], bool], converter: Callable[[T], Any], priority: float = Priority.NORMAL) -> None:
        self.predicate = predicate
        self.converter = converter
        self.priority: = priority

Is this still worth the generic? Maybe, I think it ties a Converter to its input type better, and might fix some mypy complaints. But it will introduce more, because of where we define our Java classes (i.e. in start_jvm); any Converter converting from a Java class will then be complained about because mypy cannot find the globals that will be defined later. This problem is an exacerbation of the current situation, though; mypy already complains about their usage in Converter.converters ☹️.

This might be an argument to extract the global <Java class> statements outside of the start_jvm function? If this is frowned upon in python, then we will just have to deal with the mypy errors anyways...

[ctrueden]

This might be an argument to extract the global <Java class> statements outside of the start_jvm function? If this is frowned upon in python

Certainly, Python does not frown on declaring global variables outside of functions. Of course, you don't need the global keyword if you're outside a function.

But are you suggesting you would do some scijava.jimport commands in the global scope for a module? This would be bad, because jimport implicitly starts the JVM as needed. And we must not start the JVM upon import imagej or import scyjava. It needs to happen in as deferred a way as possible.

[gselzer]

But are you suggesting you would do some scijava.jimport commands in the global scope for a module? This would be bad, because jimport implicitly starts the JVM as needed. And we must not start the JVM upon import imagej or import scyjava. It needs to happen in as deferred a way as possible.

No, I was just suggesting we could move the variable declaration outside of the function; of course the jimport calls would have to stay inside that function.

Although, honestly, I'm not sure that would fix the mypy errors anyways...

[hinerm]

To start, we actually don't want Converter.predicate to be a Callable[[T], bool], we want it to be a Callable[[Any], bool]; this function should be analogous to Converter.canConvert()

oh of course.. that makes total sense. It's not worth generifying if there aren't actually params to tie together!

[gselzer]

Great.

Anything else you'd like to change @hinerm, or shall we poke @ctrueden for merge?

[hinerm]

Anything else you'd like to change @hinerm, or shall we poke @ctrueden for merge?

Nope I'm good 👍