[ENH] add utility function to compute expected value

[Remi-Gau]

This Pull Request is related to issue: None.

Before reviewing this Pull Request, please review Pull Request [#](add a link to the Pull Request here).

Changes proposed in this Pull Request:

• adds a utility function compute the expected value for certain lists of gains and losses
• function can work with a dict, pandas dataframe or TSV file as input
• comes with tests

Checklist:

• Descriptive title
• Targets the main branch
• Changes are functional
• My code is explicit and comments were added to it
• Code conforms with PEP8
• Tests were added for the changes and they complete successfully
• Existing tests were updated (if needed) and they complete successfully
• Documentation was updated

[Remi-Gau]

@bclenet
discussing with @elodiegermani we figured that such utility function would be good as many teams used this kind of info in their stats model

however I am not sure where to put such function in the code base and on the best way to document it to make it easy to find for new contributors: any preference?

same there are some default hard coded values in that function: some teams may have used different values, so it may be useful to pass those as arguments.

would not mind to have the take of some of the original authors of the NARPS paper for the default values of the computation of the expected value.

[bclenet]

Hi @Remi-Gau, thanks for the idea !

About where to put the code, I think the narps_open.utils module is the right place. So you could write it inside the narps_open/utils/__init__.py file.
You can also put your test inside the TestUtils class, and add :

@staticmethod
@mark.unit_test

on top of it.

About the documentation, you could write a paragraph in docs/pipelines.md. If there is too much content, create a docs/utils.md file and refer to it in the docs/pipelines.md.

What do you think ?

[Remi-Gau]

What do you think ?

will do all of this. 🚀

[Remi-Gau]

Made the changes mentioned in #74 (comment)

Tried to enforce PEP8 by using flake8 but I am getting a lot or errors not due to the code I changed: it may be a good thing to have some way to enforce this a bit more.

[bclenet]

Hi and thanks for the changes,

About PEP8, what kind of errors do you get ? On which file(s) ?

[Remi-Gau]

On the things in this PR:

$ flake8 narps_open/utils/__init__.py narps_open/utils/utils.py tests/utils/test_utils.py 

narps_open/utils/__init__.py:9:1: F401 'os.path.splitext' imported but unused
narps_open/utils/__init__.py:30:22: E231 missing whitespace after ','
narps_open/utils/__init__.py:30:26: E231 missing whitespace after ','
narps_open/utils/__init__.py:30:30: E231 missing whitespace after ','
narps_open/utils/__init__.py:30:34: E231 missing whitespace after ','
narps_open/utils/__init__.py:30:38: E231 missing whitespace after ','
narps_open/utils/__init__.py:30:42: E231 missing whitespace after ','
narps_open/utils/__init__.py:30:46: E231 missing whitespace after ','
narps_open/utils/__init__.py:31:42: E228 missing whitespace around modulo operator
narps_open/utils/__init__.py:36:1: E302 expected 2 blank lines, found 1
narps_open/utils/__init__.py:56:1: E302 expected 2 blank lines, found 1
narps_open/utils/__init__.py:70:56: E251 unexpected spaces around keyword / parameter equals
narps_open/utils/__init__.py:70:58: E251 unexpected spaces around keyword / parameter equals
narps_open/utils/__init__.py:74:1: E302 expected 2 blank lines, found 1
narps_open/utils/__init__.py:87:1: E302 expected 2 blank lines, found 1
narps_open/utils/__init__.py:129:1: E302 expected 2 blank lines, found 1
narps_open/utils/__init__.py:168:1: E302 expected 2 blank lines, found 1
narps_open/utils/__init__.py:243:18: W292 no newline at end of file

But running flake8 on main on the narps_open and tests gives lot of errors:

933   E101 indentation contains mixed spaces and tabs
63    E117 over-indented
19    E124 closing bracket does not match visual indentation
7     E125 continuation line with same indent as next logical line
139   E127 continuation line over-indented for visual indent
400   E128 continuation line under-indented for visual indent
2     E129 visually indented line with same indent as next logical line
1     E131 continuation line unaligned for hanging indent
164   E203 whitespace before ':'
7     E222 multiple spaces after operator
24    E225 missing whitespace around operator
1     E228 missing whitespace around modulo operator
79    E231 missing whitespace after ':'
1933  E251 unexpected spaces around keyword / parameter equals
92    E261 at least two spaces before inline comment
16    E265 block comment should start with '# '
39    E266 too many leading '#' for block comment
2     E271 multiple spaces after keyword
9     E272 multiple spaces before keyword
1     E275 missing whitespace after keyword
131   E302 expected 2 blank lines, found 1
36    E303 too many blank lines (2)
4     E305 expected 2 blank lines after class or function definition, found 1
3     E306 expected 1 blank line before a nested definition, found 0
547   E501 line too long (109 > 100 characters)
4     E502 the backslash is redundant between brackets
2     E731 do not assign a lambda expression, use a def
1     E999 IndentationError: unexpected indent
134   F401 'nipype.interfaces.spm.Reslice' imported but unused
29    F811 redefinition of unused 'os' from line 14
9     F821 undefined name 'mask_file'
22    F841 local variable 'extract_epi' is assigned to but never used
933   W191 indentation contains tabs
756   W291 trailing whitespace
2     W292 no newline at end of file
520   W293 blank line contains whitespace
5     W391 blank line at end of file

See this CI workflow on my main branch:

https://github.com/Remi-Gau/narps_open_pipelines/actions/runs/6276436375/job/17046113029#step:5:1

[Remi-Gau]

The pep8 should be a separate issue, no? We are getting side tracked.

[bclenet]

Yep, I'll start a new issue about the PEP8 errors you mention, and PEP8 management/guidelines/doc in general.
Two quick things about that (to keep track of my thoughts until then) :

• It seems that flake8 get errors that are not raised by pylint (?!)
• Most of the errors raised by PEP8 checks on the project are due to non refactored pipelines (this is quite understandable as these files are the largest in the project, hence more prone to errors) ; it is the reason why the code_quality workflow is really permissive for now.

[Remi-Gau]

Yep, I'll start a new issue about the PEP8 errors you mention, and PEP8 management/guidelines/doc in general.

Awesome! You rock!! 👍🏾

Two quick things about that (to keep track of my thoughts until then) :

* It seems that flake8 get errors that are not raised by pylint (?!)

yeah I am not familiar with pylint but happy to only use pylint as long as there is a config file for it in the repo.

somehow I am not surprised that they do not report exactly the same errors, I suspect that if we ran ruff, we may even find some other errors...

* Most of the errors raised by PEP8 checks on the project are due to non refactored pipelines (this is quite understandable as these files are the largest in the project, hence more prone to errors) ; it is the reason why the `code_quality` workflow is really permissive for now.

ha yeah good point: adding a config to ignore them until refactored could be an approach

[Remi-Gau]

turning to draft and will probably close as "premature optimization" (preventing "don't repeat yourself" situations too early)

@bclenet if you see anything in this PR that should be salvaged let me know

[bclenet]

Hi @Remi-Gau, indeed you can let that PR as a draft to keep track of it. Thanks !

[bclenet]

Hi @Remi-Gau,
I added new modules under narps_open.core to host useful functions to write pipelines. It is a step further in code factorization... trying to keep a low impact on the pipelines themselves.
I think the expected value feature could fit there.

If it still makes sense, I'll add a method for that matter in the module.