ME-ICA / mapca

A Python implementation of the moving average principal components analysis methods from GIFT
GNU General Public License v2.0
6 stars 8 forks source link

Updating style and python versions #64

Closed handwerkerd closed 9 months ago

handwerkerd commented 9 months ago

Closes #63

The Flake8 issues were slightly more annoying than just running Black. Most were things like removing capital letters from variable names and cleaning up docstrings. Most variable name changes were simple, but one was confusing. Lambda needed the capital letter removed, but lamba has other meanings so I changed it to lamba_var. Also I had to change all print statements with .format() or % to f-strings.

There are two style issues I haven't solved yet:

codecov-commenter commented 9 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e91e16f) 90.96% compared to head (043a9d2) 90.96%.

Files Patch % Lines
mapca/mapca.py 90.90% 2 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #64 +/- ## ======================================= Coverage 90.96% 90.96% ======================================= Files 3 3 Lines 310 310 Branches 32 32 ======================================= Hits 282 282 Misses 14 14 Partials 14 14 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

eurunuela commented 9 months ago

Thank you @handwerkerd.

I will have a look later today.

eurunuela commented 9 months ago

Okay, so I don't know how we can fix the circular import issue to avoid using the relative import.

Do you think it's a critical change @handwerkerd?

I have already removed the unused img variable from transform().

handwerkerd commented 9 months ago

I'm not sure. Locally I'm seeing the error Relative import found Flake8(ABS101). It's passing the CircleCI style_check so we don't need to solve this now, but it might be an issue in the near-ish future. tedana doesn't have anything like this in its __init__.py so maybe there's a way it's not necessary. When I just got rid of it, problems arose, so it's not that simple.

In checking stuff, I also noticed I didn't update the testing files for style errors & that's fixed in the most recent push (along with added _version.py to .gitignore

handwerkerd commented 9 months ago

I do see one issue locally that might need to be resolved. In my various messing with things, mapca is not generating the correct output version info. mapca._version=='0+unknown'

When I run pip install -e .'[all]' it generates a new _version.py file but something isn't working.

eurunuela commented 9 months ago

Okay, I just realized both the relative import and the versioning issues were very silly mistakes.

I noticed we couldn't just remove the dot from the relative import, because it would be trying to import the mapca/ folder. Instead, we had to aim for the mapca.py in that folder: import mapca.mapca. Like I said, a silly mistake.

Regarding the versioning, a similar mistake was made. When importing __version__ we were targeting mapca and not the _version.py file, which is the one that actually contains the version.

Anyway, those two issues are now fixed. I will approve the PR and will let you merge if you think it's ready.