darts icon indicating copy to clipboard operation
darts copied to clipboard

First mypy refactorings

Open rijkvandermeulen opened this issue 3 years ago • 2 comments

Current situation

I did a first investigation into the existing mypy errors and the amount of refactoring that would be required. Running mypy on the whole directory results in over 650 "errors". Some of these errors are super easy to fix, others will probably require slightly more effort.

Proposal

Curious to learn how you guys look at this, but in my opinion fixing >650 errors is quite a substantial effort for one PR. I'm therefore wondering whether it would make more sense to split this up into more "manageable" chunks? The way of working would then look something like:

  • I've created a mypy config file (mypy.ini) where we a.o. ignore the mypy errors of the part of the repo that we did not refactor yet.
  • This allows us to already add mypy to the pre-commit hooks as I did as part of this PR (since it will only check the part of the repo that we actually expect to comply with mypy).
  • Having this set-up in place, we can create seperate tickets to address individual parts of the repo (e.g., we refactor darts/timeseries/py and after that we remove: [mypy-darts.timeseries.*] ignore_errors = True. This would allow us to step by step improve.

To make all of this more practical I've already took a shot on resolving the mypy issues for darts/dataprocessing/dtw and darts/dataprocessing/pipeline.py. As you'll see in the mypy.ini these directories/files are thus excluded from the "exception list" and hence mypy will check these files after each commit.

Looking forward to hearing your thoughts on this approach!

rijkvandermeulen avatar Sep 11 '22 14:09 rijkvandermeulen

Codecov Report

Base: 93.74% // Head: 93.72% // Decreases project coverage by -0.02% :warning:

Coverage data is based on head (057297a) compared to base (28ca88d). Patch coverage: 96.77% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1204      +/-   ##
==========================================
- Coverage   93.74%   93.72%   -0.03%     
==========================================
  Files          83       83              
  Lines        8407     8396      -11     
==========================================
- Hits         7881     7869      -12     
- Misses        526      527       +1     
Impacted Files Coverage Δ
darts/dataprocessing/dtw/window.py 85.71% <93.75%> (-0.37%) :arrow_down:
darts/dataprocessing/dtw/cost_matrix.py 67.03% <100.00%> (ø)
darts/dataprocessing/dtw/dtw.py 94.16% <100.00%> (-0.05%) :arrow_down:
darts/dataprocessing/pipeline.py 86.84% <100.00%> (ø)
darts/timeseries.py 92.23% <0.00%> (-0.07%) :arrow_down:
...arts/models/forecasting/torch_forecasting_model.py 87.45% <0.00%> (-0.05%) :arrow_down:
darts/models/forecasting/block_rnn_model.py 98.24% <0.00%> (-0.04%) :arrow_down:
darts/models/forecasting/nhits.py 99.27% <0.00%> (-0.01%) :arrow_down:
darts/datasets/__init__.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar Sep 12 '22 14:09 codecov-commenter

Thanks @rijkvandermeulen. It looks like a good way to do it. We'll try to review it sometime soon (we are a bit behind these days, apologies). We also will have to think whether the benefits (better type checks) over-weight the small cost (tracking all folders and dependencies in the ignore list, maintain the pre-commit hook). In any case I like your step-by-step approach!

hrzn avatar Sep 15 '22 20:09 hrzn