navis icon indicating copy to clipboard operation
navis copied to clipboard

In the long term: make interfaces stricter

Open schlegelp opened this issue 3 years ago • 10 comments

The goal is to make navis' behaviour more strict for use in a server context while staying flexible in user-facing functions.

@clbarnes do you have an example for a function that could do with a stricter interface?

schlegelp avatar Apr 15 '21 14:04 schlegelp

I think I'd want to reduce the use of globals where possible, as in a library would be harder to guarantee the config is done first.

For example, using a more standard logging setup where the modules each emit messages to their own loggers (rather than a global navis one) and leave it up to the caller to add log handlers. The existing config is great for scripting use, and in the short not-breaking-things term it could just be skipped using an environment variable like the existing headless mode. In the breaking-things term, the existing config could be in a navis.config.setup_logging() function which scripters could choose whether to call.

I'll have a closer look when I have a chance!

clbarnes avatar Apr 15 '21 17:04 clbarnes

I'll just throw ideas into this issue as I think of them - some of them may be subjective and/or overly nitpicky so feel free to ignore!

Looking at the neuron cutting methods, which can return either a TreeNeuron or a (TreeNeuron, TreeNeuron) depending on the input - I personally prefer for output signatures to be invariant so that they always unpack the same way. In this case, it would be (Optional[TreeNeuron], Optional[TreeNeuron]). I know that numpy does the opposite in a number of functions so this is probably just personal preference. On a similar theme, there's a few if inplace: return None which could just be skipped - nothing wrong with returning the mutated input argument.

I'm not a big fan of magic strings to represent enum variants, and indeed at my request python 3.10 is going to include StrEnums which are proper enums but compatible with methods which take str in this way - easier to type, document, and discover. I wrote a backport for 3.8.6+, but there are largely compatible options (like https://github.com/irgeek/StrEnum ) which support lower versions too. But magic strings will continue to be used all over python forever so again, it's not like it's much of an issue.

clbarnes avatar Apr 20 '21 16:04 clbarnes

Yeah just keep 'em coming!

On a similar theme, there's a few if inplace: return None which could just be skipped - nothing wrong with returning the mutated input argument.

Fair - that's probably a good change. It's straight forward but would touch a lot of functions.

Looking at the neuron cutting methods, which can return either a TreeNeuron or a (TreeNeuron, TreeNeuron) depending on the input

Are you referring here to cut_neuron vs e.g. prune_distal_to?

schlegelp avatar Apr 21 '21 08:04 schlegelp

Are you referring here to cut_neuron vs e.g. prune_distal_to?

Ah nvm: you mean cut_neuron with ret='distal' vs ret='proximal' vs ret='both'?

schlegelp avatar Apr 21 '21 08:04 schlegelp

FYI:

  • fef8515ce31e968735e1ec6f8d9e48bcb0b44c87 makes it so that cut_neuron always returns a single neuronlist containing 1 to many neurons (sorted distal -> proximal)
  • with d582212af60d733dcd259f717b0f0506ab16cd78 all functions should return a neuronobject even if inplace=True

schlegelp avatar Apr 29 '21 10:04 schlegelp

Great, looking forward to using them!

Another possible minor thing to look at is functions which take an "auto" string argument, which is different to None, and is also different to taking string arguments of other values. A pattern I've seen for this is

AUTO = object()

def fn(arg=AUTO):
    if arg is AUTO:  # identity rather than equivalence check
        ...
    ...

It's unlikely that anyone has e.g. a score matrix which is a file called ./auto, but it could happen, and it helps keep the mental model of the function's arguments in line with the type system.

clbarnes avatar Apr 29 '21 10:04 clbarnes

This is in 3.10 and may help out type checkers for functions which work on multiple different types: https://www.python.org/dev/peps/pep-0647/

clbarnes avatar May 07 '21 09:05 clbarnes

Cool! Do you know if this will be back ported?

schlegelp avatar May 07 '21 09:05 schlegelp

I suspect it'll work with any python once there have been a couple more mypy releases, but they'll have to be defined as strings for older pythons.

The way backporting works with typing is pretty weird because it doesn't change anything at runtime; it's just the stdlib saying "we recognise that some other people build type checkers; here is an interface those type checkers can build off". A change got pulled out of 3.10 because it was fundamentally incompatible with pydantic, for example. If the type checkers support the older versions, and the annotations don't break at runtime, I don't see why it wouldn't work.

clbarnes avatar May 07 '21 09:05 clbarnes

Yeah, I've heard about the pydantic/fastapi thing. Let's see how this pans out.

schlegelp avatar May 07 '21 09:05 schlegelp