Skip to content

135 contact reduction npi local #284

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

135 contact reduction npi local #284

wants to merge 6 commits into from

Conversation

DavidKerkmann
Copy link
Member

Changes

Implements tool to compute NPI reduction for any given county.
What can be improved:

  • Age resolved data for each county. For now, 'cases_all_county_age.json' is used because it contains the required age resolution.
  • Read-in of NPI is not implemented. For now, the NPI can be hard-coded in the file. Once we have some fixed NPI object structure and a way to export them in some way, this can be implemented.

Merge Request - GuideLine Checklist

Guideline to check code before resolve WIP and approval, respectively.
As many checkboxes as possible should be ticked.

Checks by code author:

  • There is at least one issue associated with the pull request.
  • The branch follows the naming conventions as defined in the git workflow.
  • New code adheres with the coding guidelines
  • Tests for new functionality has been added
  • A local test was succesful
  • There is appropriate documentation of your work. (use doxygen style comments)
  • If new third party software is used, did you pay attention to its license? Please remember to add it to the wiki after successful merging.
  • If new mathematical methods or epidemiological terms are used, has the glossary been updated ? Did you provide further documentation ?
    is present or referenced. Please provide your references.
  • The following questions are addressed in the documentation*: Developers (what did you do?, how can it be maintained?), For users (how to use your work?), For admins (how to install and configure your work?)
  • For documentation: Please write or update the Readme in the current working directory!

Checks by code reviewer(s):

  • Is the code clean of development artifacts e.g., unnecessary comments, prints, ...
  • The ticket goals for each associated issue are reached or problems are clearly addressed (i.e., a new issue was introduced).
  • There are appropriate unit tests and they pass.
  • The git history is clean and linearized for the merge request.
  • Coverage report for new code is acceptable.

closes #135

@DavidKerkmann DavidKerkmann added class::feature A feature to be implemented for some part of the software loc::backend This issue concerns the C++ backend implementation. loc::data analysis This issue concerns any kind of data analysis. model::ode This issue concerns any kind of ODE-based model. labels May 12, 2022
@DavidKerkmann DavidKerkmann requested a review from mknaranja May 12, 2022 08:58
@DavidKerkmann DavidKerkmann marked this pull request as ready for review May 12, 2022 08:59
@dabele dabele mentioned this pull request May 12, 2022
Closed
14 tasks
@mknaranja mknaranja changed the title 257 contact reduction npi local 135 contact reduction npi local Oct 4, 2022
@mknaranja
Copy link
Member

I changed the title to 135 as this is related to issue #135 (and not issue #257)

mknaranja
mknaranja reviewed Sep 25, 2023
View reviewed changes
tools/compute_npi_reduction.py Outdated
@return Data path of memilio repository.
"""

data_path = os.getcwd().split('memilio')[0] + 'memilio/data/'
Copy link
Member

Choose a reason for hiding this comment

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

In #319 it was discussed the problem that the directory does not need to be named memilio.

You mention this problem in the comment but maybe a better solution would be to allow the user to pass an arbitrary directory instead of always calling this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I implemented a possibility to include a path.

tools/compute_npi_reduction.py Outdated
Comment on lines 75 to 76
# TODO: We currently don't have age-resolved data for each county.
# Once we have this data, find correct file and adapt extraction to that file format.
Copy link
Member

Choose a reason for hiding this comment

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

please proceed with get_population_data()

also, you can now use the inter/extrapolation of data to new age groups using #363

Copy link
Member Author

Choose a reason for hiding this comment

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

I now use the age group transformation and the correct file. Would you prefer to use the download scripts here instead of using the already downloaded file?

@codecov
Copy link

codecov bot commented Oct 12, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.98%. Comparing base (aaacb23) to head (2aba838).
Report is 134 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #284   +/-   ##
=======================================
  Coverage   94.98%   94.98%           
=======================================
  Files         112      112           
  Lines        8776     8776           
=======================================
  Hits         8336     8336           
  Misses        440      440

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

@mknaranja
Copy link
Member

My comments dated from long ago, I forgot to submit them. I think the script is still useful and important to compute estimated total contact reduction. I am not sure though if it is correct. We would need to look at it again. In the future, one could think of integrating a similar output directly to C++.

DavidKerkmann
DavidKerkmann commented Oct 12, 2023
View reviewed changes
Copy link
Member Author

@DavidKerkmann DavidKerkmann left a comment

Choose a reason for hiding this comment

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

I resolved the comments.

tools/compute_npi_reduction.py Outdated
@return Data path of memilio repository.
"""

data_path = os.getcwd().split('memilio')[0] + 'memilio/data/'
Copy link
Member Author

Choose a reason for hiding this comment

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

I implemented a possibility to include a path.

tools/compute_npi_reduction.py Outdated
Comment on lines 75 to 76
# TODO: We currently don't have age-resolved data for each county.
# Once we have this data, find correct file and adapt extraction to that file format.
Copy link
Member Author

Choose a reason for hiding this comment

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

I now use the age group transformation and the correct file. Would you prefer to use the download scripts here instead of using the already downloaded file?

@mknaranja mknaranja requested a review from HenrZu February 7, 2024 09:26
@mknaranja
Copy link
Member

@HenrZu We should discuss how to continue and output contact reduction for ODE model summary statistics

@mknaranja
Copy link
Member

mknaranja commented Oct 16, 2024 •
edited
Loading

@HenrZu I think an output on this information is important. Maybe even better to directly do this in cpp. I close this here. Could you address this in #135?

@mknaranja mknaranja closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mknaranja mknaranja mknaranja left review comments

@HenrZu HenrZu Awaiting requested review from HenrZu

Assignees
No one assigned
Labels
class::feature A feature to be implemented for some part of the software loc::backend This issue concerns the C++ backend implementation. loc::data analysis This issue concerns any kind of data analysis. model::ode This issue concerns any kind of ODE-based model.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add tool to compute NPI contact reduction
2 participants
@DavidKerkmann @mknaranja