Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type hint fix #46

Merged
merged 51 commits into from
Oct 31, 2024
Merged

Type hint fix #46

merged 51 commits into from
Oct 31, 2024

Conversation

Kieran-Fishwick-TfN
Copy link
Contributor

@Kieran-Fishwick-TfN Kieran-Fishwick-TfN commented Sep 13, 2024

Describe Changes

updated multi tld API

Task Checklist

  • Have unittests been added (testing individual functions and their edge cases)
  • Have integration tests been added (if applicable, to test modules of functionality)
  • Updated RELEASE.md to reflect changes in PR
  • Is the documentation building correctly with a clean version number?
  • Have new dependencies been added?
    • Have they been added to either requirements.txt or requirements_dev.txt.
    • Have they been added to setup.cfg

@MattBuckley-TfN MattBuckley-TfN marked this pull request as ready for review October 18, 2024 14:38
Copy link
Contributor

@MattBuckley-TfN MattBuckley-TfN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the methodology looks good, I've mainly made comments around cleaning up the code and adding documentation.

Some more general things that need updating are:

  • Running isort (specifically on multi_area)
  • Fixing pylint issues, many of these are covered by comments for tidying up.
  • A bunch of tests are failing due to removing the params argument from MultiAreaGravityModelCalibrator these can just be updated to reflect the API change
  • One test is failing due to init params outside the new boundaries for tanner, the init parameter for the test is 4.7... and -1.6..., potentially just bringing those init params within the limits will solve the issue with the test (tests\gravity_model\data\small_and_simple\tanner\calibrate\init_params.json)

There's one additional comment on the code which I couldn't make because you haven't edited that line.
Line 726 in multi_area.py: multi_area.gravity_model function has the wrong type annotation for cost_distributions should be MultiCostDistribution not list[MultiCostDistribution]

.gitignore Outdated Show resolved Hide resolved
src/caf/distribute/gravity_model/core.py Outdated Show resolved Hide resolved
src/caf/distribute/gravity_model/core.py Outdated Show resolved Hide resolved
src/caf/distribute/gravity_model/core.py Show resolved Hide resolved
src/caf/distribute/gravity_model/core.py Outdated Show resolved Hide resolved
src/caf/distribute/gravity_model/multi_area.py Outdated Show resolved Hide resolved
src/caf/distribute/gravity_model/multi_area.py Outdated Show resolved Hide resolved
src/caf/distribute/gravity_model/multi_area.py Outdated Show resolved Hide resolved
src/caf/distribute/gravity_model/multi_area.py Outdated Show resolved Hide resolved
src/caf/distribute/gravity_model/multi_area.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 26 lines in your changes missing coverage. Please review.

Project coverage is 85.29%. Comparing base (33cd564) to head (269942c).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/caf/distribute/gravity_model/core.py 33.33% 14 Missing ⚠️
src/caf/distribute/gravity_model/multi_area.py 87.23% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
- Coverage   86.31%   85.29%   -1.03%     
==========================================
  Files          11       11              
  Lines         994     1047      +53     
==========================================
+ Hits          858      893      +35     
- Misses        136      154      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@MattBuckley-TfN MattBuckley-TfN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of previous comments haven't been addressed and one new one about a TODO that I think has been completed

src/caf/distribute/gravity_model/multi_area.py Outdated Show resolved Hide resolved
@Kieran-Fishwick-TfN Kieran-Fishwick-TfN merged commit b53ca8a into main Oct 31, 2024
15 checks passed
@MattBuckley-TfN MattBuckley-TfN deleted the type-hint-fix branch October 31, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants