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

Include ntdmc output date #36

Merged
merged 13 commits into from
Jan 15, 2025
Merged

Include ntdmc output date #36

merged 13 commits into from
Jan 15, 2025

Conversation

mattg3004
Copy link
Collaborator

the output of IHME and NTDMC data has been linked in the code up to this point, not allowing us to output all of the NTDMC data without outputting all of the IHME data. Include an indicator and date of NTDMC outputs, so that we can separate these two data sets and allow us to output them from different dates, or output one without the other.

@mattg3004 mattg3004 requested a review from thk123 January 9, 2025 06:21
Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turn around and looks like it works! Couple of small blocking comments (documenting new flags, fixing a comment that confuses the situation) and some suggestions if you feel inclined

src/Model.cpp Outdated Show resolved Hide resolved
@@ -27,7 +27,8 @@ extern Statistics stats;
void Model::runScenarios(ScenariosList &scenarios, Population &popln,
Vector &vectors, Worm &worms, int replicates,
double timestep, int index, int outputEndgame,
int outputEndgameDate, int reduceImpViaXml,
int outputEndgameDate, bool outputNTDMC,
Copy link
Contributor

Choose a reason for hiding this comment

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

💚 This is good to be using boolean rather than an int with the others, makes it clear what the intention of the variable is, and prevents passing muddling up the two parameters.

One neat way to encode this common pair (of some data, and a boolean flag to say whether to use the data) is std::optional:

Suggested change
int outputEndgameDate, bool outputNTDMC,
int outputEndgameDate, std::optional<int> outputNTDMCDate,

And then when you use it you can do:

if(outputNTDMCDate.hasValue()){
  outputNTDMCDateInYears = (outputNTDMCDate.value - BASEYEAR) * 12;
}

But no need to do that for now.

src/Model.cpp Outdated Show resolved Hide resolved
src/main.cpp Show resolved Hide resolved
src/Model.cpp Outdated Show resolved Hide resolved
@thk123
Copy link
Contributor

thk123 commented Jan 9, 2025

I've removed the unsupported Mac job from CI - so if you merge main into this you'll get a green tick - but since it is only that job that is failing I don't think you need to.

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes - let me know if you would like help with getting the linux build working, not quite sure how your changes could have caused it

README.md Outdated

* -D 2026: year from which to output IHME data from. Change to whatever year we want to do this from. Default is 2000.

* -m 0 : indicator for outputting NTDMC data. If this is omitted it will be output. If set to 0, this will not be done. Any other integer input will lead to it being output.
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Use the same sentence structure as for -e:

* -m 1 or 0 (1 is default): set to 0 if we want don't want to output the NTDMC data.

(I don't think we need to document that they can also provide 42)

@thk123
Copy link
Contributor

thk123 commented Jan 14, 2025

AFAICT this is failing now because GH have updated "ubuntu-latest" to 24.04, and this is failing because GSL is not installed (I am surprised it is installed on 22.04 - I'll raise a PR fixing this.

@thk123
Copy link
Contributor

thk123 commented Jan 14, 2025

Fixed in #38 - I'll post here when it is merged. Clearly this PR is cursed 😆

@thk123
Copy link
Contributor

thk123 commented Jan 14, 2025

#38 is merged, so if you merge main into this CI should pass 👍

@mattg3004 mattg3004 merged commit d24650f into main Jan 15, 2025
6 checks passed
@mattg3004 mattg3004 deleted the includeNTDMCOutputDate branch January 15, 2025 02:55
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