Review

[ZviBaratz]

Hi everyone,

Great job on your group project! :tada:

---

:sparkles: README and basic usage instructions. :sparkles: Using PRs to collaborate on a shared codebase. :sparkles: Installable Python package. :sparkles: Scripts are executable and have the expected output format. :sparkles: The code is organized and broken down into modules of reasonably sized functions. :sparkles: Basic tests and passing CI.

---

Here are some notes from looking at the repo:

• Would be great if you open issues before submitting the PRs to resolve them. It creates a clean workflow that is easy to follow and reference.
• ~For string templates such as this:~ https://github.com/nipraxis-spring-2022/diagnostics-Lemanics/blob/27bcf18e034feebf3de3ae1608210b2f649fa03b/findoutlie/data_load.py#L40 ~Consider using constants, e.g.:~

  TEMPLATE = "sub-{subject_id}_task-taskzero_run-{i_run}_bold.nii.gz"
  ...
  name = TEMPLATE.format(subject_id=subject_id, i_run=i_run)

  ~This will help minimize clutter within functions, improve reusability, and facilitate long-term code maintenance.~

• Prefer using pathlib over os.path.
• Use isinstance() rathen then type(x) == type, e.g. in: https://github.com/nipraxis-spring-2022/diagnostics-Lemanics/blob/27bcf18e034feebf3de3ae1608210b2f649fa03b/findoutlie/data_load.py#L73 it would be better to have:

  if isinstance(sub_id, int):

• Avoid reimplementing built-in functionality, e.g.: https://github.com/nipraxis-spring-2022/diagnostics-Lemanics/blob/27bcf18e034feebf3de3ae1608210b2f649fa03b/findoutlie/data_load.py#L85-L86 and https://github.com/nipraxis-spring-2022/diagnostics-Lemanics/blob/27bcf18e034feebf3de3ae1608210b2f649fa03b/findoutlie/data_load.py#L115-L116 Python will raise a FileNotFoundError and specify the path of the file not found, so reimplementing it only adds to the complexity of your code with no benefit to its functionality.
• Consider replacing your usage of globals() to point to functions with a dispatch table. For exmaple, instead of: https://github.com/nipraxis-spring-2022/diagnostics-Lemanics/blob/27bcf18e034feebf3de3ae1608210b2f649fa03b/findoutlie/detectors.py#L33 it would be better to have something like:

  
  def iqr_detector(*args, **kwargs):
  ...

def median_detector(*args, **kwargs): ...

DETECTORS = { "IQR": iqr_detector, "median": median_detector, }

def compute_outliers(detector: str = "IQR", *args, **kwargs): detector_func = DETECTORS[detector] ....

Generally speaking, avoid relying on `globals()` to access (let alone change) anything.

@matthew-brett

[celprov]

Thanks a lot @ZviBaratz for all those useful comments. I have a question out of curiosity : why should we prefer pathlib over os.path ?

[ZviBaratz]

Hi @celprov, thank you. I'm glad you found it useful.

Regarding pathlib, this is merely a recommendation - pathlib is more modern and more specialized (i.e. oriented specifically to facilitate filesystem path operations), while the os module is more low-level and general-purpose. There are numerous operations that are much simpler to execute using pathlib and will result in more readable code.

I also wanted to take back my comment regarding string templating (using format over f-strings), this is more of a personal preference, really.