Move logging.py inside utils

niksirbi commented 2 weeks ago

Description

What is this PR

[ ] Bug fix
[ ] Addition of a new feature
[x] Other (refactoring)

Why is this PR needed?

The functions in logging.py are in effect used by developers/contributors, as such I thinks it makes more sense to put them under movement.utils rather than have them as a top-level module.

What does this PR do?

References

If this gets merged, PR #213 needs to be updated.

How has this PR been tested?

Existing tests still pass.

Is this a breaking change?

I guess a bit? But not really affecting anything user-facing.

Does this PR require an update to the documentation?

Yes, the API index has been modified accordingly.

Checklist:

[x] The code has been tested locally
[ ] Tests have been added to cover all new functionality
[x] The documentation has been updated to reflect any changes
[x] The code has been formatted with pre-commit

sonarcloud[bot] commented 2 weeks ago

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.68%. Comparing base (0114b91) to head (83dfe4d).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #216 +/- ## ======================================= Coverage 99.68% 99.68% ======================================= Files 12 12 Lines 643 643 ======================================= Hits 641 641 Misses 2 2 ```

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

niksirbi commented 2 weeks ago

I think we anticipated that we would definitely end up with more modules in analysis and utils, even if they only had 1 module to begin with. I'm not sure what's the best strategy here. On the one hand, it makes sense to start flat and only nest when necessary, but on other hand, we want to reduce the number of breaking changes to the API, so we try to anticipate the future structure. So, to answer your question, I'm not sure.

filtering.py is a strange case. It was always meant as a placeholder, as I expected it to subdivide into smoothing.py, interpolation.py etc, in which case they would all become part of a sub-package. But we never really reached agreement on what that sub-package should be called. Some ideas were prep (for preprocessing or preparation), proc (for processing), clean etc. Conceptually I see these "filtering" functions as separate from analysis, so in my head:

if you take a variable (e.g. position), and you return the same variable but "cleaner", it's a "filter"/preprocessing step.
if you take variable(s) and derive some new quantity, e.g. velocity, or time spent in ROI, it's "analysis".

The idea here being that you normally do the "cleaning"/"prepping" before the actual analysis/statistics, as indicated in the figure below.

movement_filter_kinematics_figure

But I don't know how much sense that mental model makes, so I'm completely open to suggestions.

niksirbi commented 2 weeks ago

cli_entrypoint.py is also a special case, because it's not really a module. I'm fine for it to stay where it is right now. If we end up adding more CLI commands/script, we may consider grouping them under "cli" or "scripts".

sfmig commented 2 weeks ago

sounds good, I say let's keep as is for now and we can adapt when/if this becomes cumbersome or a problem

neuroinformatics-unit / movement