NeurodataWithoutBorders / pynwb

A Python API for working with Neurodata stored in the NWB Format
https://pynwb.readthedocs.io
Other
173 stars 83 forks source link

[Feature]: use `__all__` for modules #1686

Open bendichter opened 1 year ago

bendichter commented 1 year ago

What would you like to see added to PyNWB?

Python allows you to use __all__ to indicate the public interface of a module. This is commonly used for the from x import * pattern, where it limits the returned classes to those listed in the __all__ list.

While I don't necessarily recommend using that import pattern, I still think __all__ would be useful, because it informs the user what public classes are defined in that module. I have a current situation where I would like to import all of the neurodata types implemented in a module. Simply using the dir() command, I get all of the neurodata types that are imported into the module, as well as a bunch of double underscore functions, which I do not want.

from pynwb import ecephys
dir(ecephys)
['CORE_NAMESPACE',
 'ClusterWaveforms',
 'Clustering',
 'DataChunkIterator',
 'Device',
 'DynamicTableRegion',
 'ElectricalSeries',
 'ElectrodeGroup',
 'EventDetection',
 'EventWaveform',
 'FeatureExtraction',
 'FilteredEphys',
 'Iterable',
 'LFP',
 'MultiContainerInterface',
 'NWBContainer',
 'NWBDataInterface',
 'SpikeEventSeries',
 'TimeSeries',
 '__builtins__',
 '__cached__',
 '__doc__',
 '__file__',
 '__loader__',
 '__name__',
 '__package__',
 '__spec__',
 'assertEqualShape',
 'docval',
 'get_data_shape',
 'get_docval',
 'popargs',
 'popargs_to_dict',
 'register_class',
 'warnings']

__all__ lists are used on many popular packages, such as sci-kit learn: https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/feature_extraction/image.py

The __all__ list could be used in a number of ways, for instance to determine coverage of tests, mock classes, and tutorials.

Is your feature request related to a problem?

No response

What solution would you like?

see above

Do you have any interest in helping implement the feature?

Yes.

Code of Conduct

CodyCBakerPhD commented 1 year ago

I will point out that a part of the reason it works really well on SKlearn is because they also have the __init__ for each sub-module well-partitioned throughout the project

We could (and probably should) do that here as well, where instead of having a single file called ecephys.py you have a folder ecephys with it's own __init__ and any number of sub-files to break up the class definitions in a more readable/searchable manner (rather than scrolling through 351 lines to find the class I'm looking for)

Also note that this entire discussion might sound like it breaks back-compatibility, but if done well (and tested well; related to #1493) it actually wouldn't even be noticeable except that auto-completion of imports contains much less noise

bendichter commented 1 year ago

@CodyCBakerPhD I think breaking modules into submodules could be a separable issue. Would you mind if we discussed that in a different thread?

oruebel commented 1 year ago

Defining __all__ sounds good to me.

I think breaking modules into submodules could be a separable issue

I agree that creating submodules should be a separate issue. I think what is being proposed here, is to have __all__ defined in ecephys.py directly (as well as all the other files, e.g., icephys.py, ophys.py etc.).

bendichter commented 1 year ago

@oruebel yes, that's right.