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.

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

Converted Param.h icdf arrays from double array to InverseCdf class #316

Merged
merged 27 commits into from
Jun 19, 2020
Merged

Converted Param.h icdf arrays from double array to InverseCdf class #316

merged 27 commits into from
Jun 19, 2020

Conversation

DavidVernest
Copy link
Contributor

@DavidVernest DavidVernest commented May 21, 2020 •
edited
Loading

Closes #315. (Refactor the ICDF arrays in Param.h / CovidSim.cpp)

  • Introduces a new class InverseCdf
  • Removes SetICDF() from CovidSim.cpp
  • Adds helper function GetInverseCdf() to replace individual calls
  • Uses InverseCdf functions set_defaults() and apply_exponent()
  • Moves ChooseFromICDF() from Update.cpp to InverseCdf function choose()

CloneDeath
CloneDeath suggested changes May 21, 2020
View reviewed changes
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/ICDF.h Outdated Show resolved Hide resolved
@CloneDeath
Copy link
Contributor

Looks good. I think it's a step in the right direction, for sure.

@DavidVernest DavidVernest changed the title Converted Param MildToRecovery_icdf from double array to ICDF class Converted Param.h icdf arrays from double array to InverseCdf wrapper class May 22, 2020
@DavidVernest DavidVernest marked this pull request as ready for review May 22, 2020 09:58
@DavidVernest DavidVernest reopened this May 22, 2020
@DavidVernest DavidVernest changed the title Converted Param.h icdf arrays from double array to InverseCdf wrapper class Converted Param.h icdf arrays from double array to InverseCdf class May 27, 2020
@ozmorph ozmorph mentioned this pull request May 28, 2020
Draft
7 tasks
DavidVernest referenced this pull request Jun 2, 2020
@NeilFerguson
More tidying
b126ae6
DavidVernest added 14 commits June 5, 2020 13:42
@DavidVernest
Converted Param MildToRecovery_icdf from double array to ICDF class
deaabde
@DavidVernest
Update CMakeLists.txt
17d3581
@DavidVernest
Use math.h
089f4ab
@DavidVernest
convert remaining ICDF variables
34ec6c0 
rename InverseCdf to guidelines
convert remaining ICDF variables
@DavidVernest
switch order of header files
85be149
@DavidVernest
convert remaining ICDF arrays to InverseCdf
f9d10bd 
rm SetICDF()
add apply_exponent(int)
add get_value(int)
@DavidVernest
change startValue parameter of InverseCdf::set_defaults from int to d…
467eb9b 
…ouble

This is to avoid data loss during an unnecessary type conversion.
@DavidVernest
replace get_value() with index operator
4f7c0c7
@DavidVernest
use snake_case for variables
c2a7257
@DavidVernest
remove use of get_value()
dc25b9e
@DavidVernest
remove class name from declaration
b1e212e
@DavidVernest
Add to CovidSim.cpp GetInverseCdf()
7ae3a01 
Move ChooseFromICDF() from Update.cpp to InverseCdf.cpp and rename to choose()
@DavidVernest
revert InverseCdf.hpp back to a .h file.
90dfaeb
@DavidVernest
Convert from .hpp to .h
782f370
@DavidVernest
Convert ICDF_START from int to double
27025b8 
The start value for ICDF arrays was hardcoded to 100. This was extracted to a const, but incorrectly given the data type of int. To avoid any implicit type conversions, this has now been specified as a double value of 100.0.
@DavidVernest DavidVernest mentioned this pull request Jun 6, 2020
Open
@DavidVernest
Copy link
Contributor Author

DavidVernest commented Jun 19, 2020 •
edited
Loading

@matt-gretton-dann @dlaydon @weshinsley PR #316 is now 29 days old. It finishes the cleanup of ICDF, incorporates Neil Ferguson's data-type fix (from int to double), and passes all tests. Would appreciate a review/merge so I can concentrate on the Update.cpp work. Thanks.

weshinsley
weshinsley reviewed Jun 19, 2020
View reviewed changes
src/InverseCdf.cpp Outdated Show resolved Hide resolved
@weshinsley weshinsley merged commit 863389f into mrc-ide:master Jun 19, 2020
@DavidVernest DavidVernest deleted the ICDF branch July 28, 2020 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weshinsley weshinsley weshinsley approved these changes

@CloneDeath CloneDeath CloneDeath approved these changes

@dlaydon dlaydon Awaiting requested review from dlaydon dlaydon is a code owner

@gnedjati gnedjati Awaiting requested review from gnedjati gnedjati is a code owner

@igfoo igfoo Awaiting requested review from igfoo igfoo is a code owner

@matt-gretton-dann matt-gretton-dann Awaiting requested review from matt-gretton-dann matt-gretton-dann is a code owner

@NeilFerguson NeilFerguson Awaiting requested review from NeilFerguson NeilFerguson is a code owner

@Z80coder Z80coder Awaiting requested review from Z80coder Z80coder is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor the ICDF arrays in Param.h / CovidSim.cpp
4 participants
@DavidVernest @CloneDeath @weshinsley @dlaydon