MartinHeroux / spike2py

spike2py provides a simple interface to analyse and visualise data
GNU General Public License v3.0
12 stars 8 forks source link

fixing imports #37

Closed bbelderbos closed 4 years ago

bbelderbos commented 4 years ago

@MartinHeroux what about this?

MartinHeroux commented 4 years ago

With this configuration, to create a trial a user would need to do the following:

import spike2py
info = spike2.trial.TrialInfo(file='fatigue.mat')
spike2py.trial.Trial(info)

As TrialInfo and Trial are the two main things needed to actually read in and process data, I can simply this by included the following two lines in the _init__.py file:

from . import signal_processing
from .trial import TrialInfo, Trial

So now the user get up and running quickly. I think this is pretty nice.

import spike2py
info = spike2.TrialInfo(file='fatigue.mat')
spike2py.Trial(info)

The only issue I have is that the from . import <> style of import brings in absolutely everything.

For example if the user were to type spike2py.channels. and hit tab, they would be faced with the following:

spk4

So the user sees all the constants, helper functions, and other packages (e.g. np, plot, namedtuple) that I imported in my module. This seems rather ugly and I don't think this type of stuff appears in real open source projects.

I tried __all__ in these modules, but I could not get it to work. For example, I tried just having it in the channels module and left the current from . import channels in init.py, but this did not work. I then changed the line in init.py to from .channels import *, but this also did not work.

Would be nice to limit the things that are visible to the user...

bbelderbos commented 4 years ago

@MartinHeroux what about prepending "private" methods / classes with a single underscore? https://stackoverflow.com/a/551361

When I do this for Channel and ChannelDetails (so changing them to: _Channel and _ChannelDetails, just for example's sake), they are effectively hidden from the interface:

>>> spike2py.channels.
spike2py.channels.Event(           spike2py.channels.NamedTuple(      spike2py.channels.parsed_keyboard( spike2py.channels.Path(            spike2py.channels.Waveform(
spike2py.channels.Keyboard(        spike2py.channels.np               spike2py.channels.parsed_waveform( spike2py.channels.plot             spike2py.channels.Wavemark(
spike2py.channels.Literal(         spike2py.channels.parsed_event(    spike2py.channels.parsed_wavemark( spike2py.channels.sig_proc
>>> spike2py.channels.
MartinHeroux commented 4 years ago

Hi bob, does this depend on the Python console being used?

Because I fear Pycharm sometimes gives me too much information on auto-complete, I tried running my package in Ipython from the terminal. And whether I import spike2py or from spike2py import channel, I get the following:

d

I notice that the underscore function don't appear, but the imported packages do show up, including all the elements from typing? I don't remember seeing numpy or matplotlib when I import pandas...

Any ideas?

bbelderbos commented 4 years ago

@MartinHeroux maybe worth looking at how pandas does it? It seems to be very explicit about imports: https://github.com/pandas-dev/pandas/blob/master/pandas/__init__.py

I wonder if your imports in __init__.py are too broad then?

more spike2py/__init__.py
from . import read
from . import channels
from . import sig_proc
from . import plot
from . import trial
from . import types

Should these be something like: from .channels import Class1, Class2, ...?

That did give you issues about testing, but I feel we can workaround that.

Maybe try this for one or two classes on a new branch and see if it narrows the interface, hiding the stuff you don't want the user to see. If it works, do it for all. Then fix the testing.

Agreed this could be for later concern. In that case just open an issue so we can track it.

HTH Bob

bbelderbos commented 4 years ago

@MartinHeroux as discussed open me a PR / branch please (and confirm encapsulation works), and I will see how to fix the tests. Thanks