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

Feature #247

Merged
merged 65 commits into from
Nov 30, 2023
Merged

Feature #247

merged 65 commits into from
Nov 30, 2023

Conversation

mghosh00
Copy link
Contributor

@mghosh00 mghosh00 commented Nov 22, 2023

Summary

Created ids for Person and Household, and changed the Cell and Microcell ids to be strings

Checklist

  • All new functions have docstrings in the correct style

  • I've verified the complete docs build locally without errors

  • I've maintained 100% coverage (please mention any 'no cover' annotations explicitly)

  • I've unit-tested all new methods directly

  • Breaking change (fix or feature that would cause existing functionality to change)

  • I have read the CONTRIBUTING document.

  • My code follows the code style of this project.

  • I have updated the wiki with new parameters/model functionality

Closing issues

Closes #242 .

@mghosh00 mghosh00 linked an issue Nov 22, 2023 that may be closed by this pull request
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0948f45) 100.00% compared to head (3edfb19) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #247   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          105       105           
  Lines         3380      3427   +47     
=========================================
+ Hits          3380      3427   +47     

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

Copy link
Contributor

@KCGallagher KCGallagher left a comment

Choose a reason for hiding this comment

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

Great work! Sorry there are always a lot of comments on the first PR, but this is great quality work, and I've just added a lot of small corrections, as well as a few wider suggestions

pyEpiabm/pyEpiabm/core/cell.py Outdated Show resolved Hide resolved
pyEpiabm/pyEpiabm/core/cell.py Outdated Show resolved Hide resolved
pyEpiabm/pyEpiabm/core/cell.py Outdated Show resolved Hide resolved
pyEpiabm/pyEpiabm/core/household.py Outdated Show resolved Hide resolved
pyEpiabm/pyEpiabm/core/household.py Outdated Show resolved Hide resolved
pyEpiabm/pyEpiabm/routine/file_population_config.py Outdated Show resolved Hide resolved
pyEpiabm/pyEpiabm/core/cell.py Outdated Show resolved Hide resolved
Co-authored-by: Kit Gallagher <[email protected]>
Copy link
Contributor

@KCGallagher KCGallagher left a comment

Choose a reason for hiding this comment

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

Almost there!

pyEpiabm/pyEpiabm/routine/file_population_config.py Outdated Show resolved Hide resolved
pyEpiabm/pyEpiabm/intervention/travel_isolation.py Outdated Show resolved Hide resolved
pyEpiabm/pyEpiabm/routine/file_population_config.py Outdated Show resolved Hide resolved
Copy link
Contributor

@KCGallagher KCGallagher left a comment

Choose a reason for hiding this comment

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

Does the most recent change not negate the need for the change_id argument in add_household?

pyEpiabm/pyEpiabm/core/microcell.py Outdated Show resolved Hide resolved
Copy link
Contributor

@KCGallagher KCGallagher left a comment

Choose a reason for hiding this comment

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

Sorry for more changes - I promise there's nothing big here - some typing points and two quick tests, but no functional changes.

pyEpiabm/pyEpiabm/intervention/travel_isolation.py Outdated Show resolved Hide resolved
pyEpiabm/pyEpiabm/core/microcell.py Outdated Show resolved Hide resolved
Copy link
Contributor

@KCGallagher KCGallagher left a comment

Choose a reason for hiding this comment

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

Brilliant work @mghosh00 and @abbie-evans on this PR - thanks for your perseverance on this and all the hard work to get this finished! I am very happy with this addition, and you both adapted well to the issues coding this up that I had not anticipated, particularly regarding travel isolation.

@mghosh00 mghosh00 merged commit a4c07c0 into main Nov 30, 2023
15 checks passed
@KCGallagher KCGallagher deleted the feature-id branch December 1, 2023 11:10
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.

ID implementation
3 participants