SainsburyWellcomeCentre / crabs-exploration

A toolkit for detecting and tracking crabs in the field.
BSD 3-Clause "New" or "Revised" License
7 stars 0 forks source link

Refactor setup of loggers #187

Closed sfmig closed 5 months ago

sfmig commented 5 months ago

This PR comes from work in PR #136 and suggests a refactoring of the setup_loggers functions that keeps the structure of train_model.py and evaluate_model.py consistent.

It defines: 1- a setup_logger_with_checkpointing function in detection_utils.py that defines a logger with checkpointing. 2- a setup_logger_without_checkpointing function in detection_utils.py for a logger without checkpointing. 3- then we import these functions in train_model.py and evaluate_model.py for their specific self.setup_logger() methods. (And we can import that in the optuna code too in the future #188 )

codecov-commenter commented 5 months ago

Codecov Report

Attention: Patch coverage is 16.66667% with 15 lines in your changes missing coverage. Please review.

Project coverage is 37.17%. Comparing base (c372e30) to head (667c7ef). Report is 2 commits behind head on main.

Files Patch % Lines
crabs/detection_tracking/detection_utils.py 17.64% 14 Missing :warning:
crabs/detection_tracking/train_model.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #187 +/- ## ========================================== + Coverage 37.12% 37.17% +0.04% ========================================== Files 19 19 Lines 1309 1310 +1 ========================================== + Hits 486 487 +1 Misses 823 823 ```

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

sfmig commented 5 months ago

good call, dunno what happened there!

Instead of a default None for ckpt_config I used an empty dict cause it allows for a shorter syntax the first time we instantiate the MLFlowLogger (and an empty dict is evaluated to false).

I also reduced things further and combined setup_mlflow_logger with the suggested setup_logger.

I'll request review again so that you can have a look before merging.