Refactoring - Githubissues

ZviBaratz commented 2 years ago

Hi BrainPrint maintainers,

I was hoping to use BrainPrint for the purposes of my PhD, and I ended up making some relatively widespread changes (in the package layout and coding style, not the analysis itself) which might prove somewhat difficult to review. It is still a WIP, but I was hoping someone would be interested in collaborating on this so that the proposed changes will eventually be merged upstream and hopefully benefit more users. I would be grateful for any thoughts or feedback in the process of working on this. I will, at some point, merge these changes to my own fork's main branch, but obviously it would be best if we managed not to diverge.

All the best, looking forward to run the analysis on my lab's dataset! Zvi

kdiers commented 2 years ago

Hi Zvi,

thanks for sharing your code improvements, that's always welcome. We'll take a closer look, and get back during the next days.

Best regards, Kersten

kdiers commented 2 years ago

Hi again,

and thanks for your suggestions, which are much appreciated. We'll try them out during the next weeks, likely in the first half of June, and get back.

Best regards, Kersten

ZviBaratz commented 2 years ago

Hi @kdiers,

Gladly. I hope you find my suggestions useful.

I will try to also update the README sometime soon. Note that after installing the package there should now be a brainprint console script available (so that there's no need to call python brainprint.py).

Best, Zvi

ZviBaratz commented 2 years ago

Hi, any news? I would be happy to help clarify any changes if that would help.

kdiers commented 2 years ago

Hi - and sorry, no news yet, although it's not forgotten. We've started looking into it earlier this month, but had to interrupt. Will try to catch up,

Kersten

m-reuter commented 2 years ago

Thank you for this pull request and your work on this. We appreciate your efforts and really like your changes with respect to the updated docstrings, the introduction of types, and the overall (toml) architecture. However there are several edits that would need to be reverted in order for this PR to become useful:

In the toml file black and flake8 (as dependency and their config) is very special to your setup and would need to be removed.
Some files appear to be deleted and recreated in another folder (e.g. Solver.py), while others (e.g. Conformal.py) are simply moved (at least on GitHub when viewing the changed files). This is problematic as the diff for these files is lost, in contrast to those that were moved. This makes it hard for us to assess any potential changes within these files. If possible treat all files the same and make sure that they are moved and diffs show correctly.
Code formatting lines: also here we allow longer line width and don’t want to break lines. Further your lint formatter breaks in strange places (leaving closing brackets on their own line etc). So please remove the automated line breaking. In some places line splits look good (e.g. in Functions with parameter initialisations) and can stay.
Code formatting quotes: also we do not care about single or double quotes. Developers can decide what they prefer. So those don't have to be changed. In fact we slightly prefer single quotes, to make it easier to use " inside (which is more common than the other way around). But we don't really care.
messages.py: similarly most messages are very specific, and should stay in their original files / functions, also to avoid too much switching between files during development. Please revert both messages and config files. There are also no plans to support other languages than english.
Your gitignore contains many paths that are special to your setup. Optimally we try to avoid polluting the code with personal settings.

Note that we did not mention the configurations.py which we like here, as some of them really are configurations (that could be changed to produce a different brainprint definition). That file is still a mix of different types of constants, but for now we can keep it.
if possible, we’d prefer less files in the CLI folder, i.e. help_text.py, parser.py, and utils.py could possibly be combined into a single file
some information seems to be lost in the revised README, such as FreeSurfer as a requirement, and available arguments / options when using the program as a python package

Thanks.

ZviBaratz commented 2 years ago

Hi, thank you for your feedback.

In the toml file black and flake8 (as dependency and their config) is very special to your setup and would need to be removed.

Please note that flake8 and black are not dependencies of the package, but additional dependencies included for development (if the package is installed with the [dev] modifier), and explicitly specifying linting and formatting tools and their configurations is a highly recommended common practice in code management. Both packages are the standard in the Python ecosystem, but if you would like to replace them with others (e.g. autopep8 and yapf) that can be done easily, but will probably still require some configurations to be specified somewhere.

Code formatting lines: also here we allow longer line width and don’t want to break lines. Further your lint formatter breaks in strange places (leaving closing brackets on their own line etc). So please remove the automated line breaking. In some places line splits look good (e.g. in Functions with parameter initialisations) and can stay.

Code formatting quotes: also we do not care about single or double quotes. Developers can decide what they prefer. So those don't have to be changed. In fact we slightly prefer single quotes, to make it easier to use " inside (which is more common than the other way around). But we don't really care.

Both preferences can be configured using black to your satisfaction and updated by running it over the src/ directory.

Your gitignore contains many paths that are special to your setup. Optimally we try to avoid polluting the code with personal settings.

The .gitignore files is shared across all developers of a package, and therefore is expected to cover commonplace setups that may vary across operating systems, development workflows, etc. E.g. including variation of virtual environment directory names (venv, .venv, .env, etc.) is expected and generally appreciated. See here and here for useful references.

Regarding the rest of the comments, unfortunately I will not have time to address these anytime soon if at all. Since most of the changes requested are a matter of styling choices and personal preference, I propose either implementing them as you'd like on this branch, or discarding this PR and diverging development for the time being. In any case, I appreciate the thoughtful review.

m-reuter commented 1 year ago

Closed by the updated refactoring in #7

Deep-MI / BrainPrint

Refactoring #5