mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

Consider using absolute imports

Open cbrnr opened this issue 1 year ago • 8 comments

I'd like to suggest to use absolute instead of relative imports. Absolute imports are more readable and in general provide better error messages, so PEP8 (and I 😄) recommend(s) using them (https://peps.python.org/pep-0008/#imports). Most of the work can probably be done by Ruff (https://docs.astral.sh/ruff/rules/relative-imports/).

cbrnr avatar Jul 11 '24 07:07 cbrnr

FWIW MNE-BIDS has been using absolute imports for many years, maybe @sappelhoff would care to share the reasoning behind and experience with that?

I do agree that the relative imports in MNE are most often very much unreadable: two, three, sometimes four leading dots .... is just nothing my brain is capable of processing efficiently

2nd FWIW: For all my own projects I'm using absolute imports in combination with a src layout. This has the positive effect that it's now basically impossible to accidentally work with a messed-up development installation (or to forget installation altogether), as imports from the non-installed packages will now immediately fail, which is not the case when using relative imports and a flat package hierarchy.

hoechenberger avatar Jul 11 '24 07:07 hoechenberger

Although this is a separate (but related) issue, I'm also +💯 on switching to a src layout. I've also used both things (absolute imports and src layout) in MNELAB and SleepECG without any problems (not surprisingly, because these are the recommended approaches nowadays).

cbrnr avatar Jul 11 '24 07:07 cbrnr

Although this is a separate issue, I'm also +💯 on switching to a src layout. I've also used both things (absolute imports and src layout) in MNELAB and SleepECG without any problems (not surprisingly, because these are the recommended approaches nowadays).

numpy, scipy, scikit-learn, joblib don't use src layout...

no objection with absolute imports

Message ID: @.***>

agramfort avatar Jul 11 '24 07:07 agramfort

Sorry about conflating this issue / request here, let's maybe not consider / discuss src layout for now and focus on absolute imports only :)

hoechenberger avatar Jul 11 '24 08:07 hoechenberger

FWIW MNE-BIDS has been using absolute imports for many years, maybe @sappelhoff would care to share the reasoning behind and experience with that?

I have always tried to recommend / enforce this (when I had the decision power), for the same reasons that Clemens mentions. :-) I would be very happy if we adopted this in MNE-Python, too.

sappelhoff avatar Jul 11 '24 09:07 sappelhoff

I have always tried to recommend / enforce this (when I had the decision power),

Yes, it was actually you who made me research this topic a little after you rejected my proposal to switch MNE-BIDS to relative imports a couple of years back or so :) this is when I figured that absolute imports are actually officially recommended.

hoechenberger avatar Jul 11 '24 10:07 hoechenberger

Okay with me to migrate to absolute imports. It'll probably create a lot of PR conflicts so we should make sure we don't have any big pending PRs open when we do it (or at least be aware of what they are so we can consciously decide to accept the rebase/merge pain)

larsoner avatar Jul 11 '24 13:07 larsoner

Okay with me to migrate to absolute imports.

no strong feeling. When doing so we should update our import/nesting tests to detect and fail if anyone puts in a relative import.

drammock avatar Jul 11 '24 16:07 drammock