Skip to content

Conversation

@kzscisoft
Copy link
Contributor

Throws an exception if the container is size zero, prevents seg fault. This covers the case of the model being run without the data being available.

Throws an exception if the container is size zero, prevents seg fault
@kzscisoft kzscisoft requested a review from a team August 25, 2020 13:49
Copy link
Contributor

@peter-t-fox peter-t-fox left a comment

Choose a reason for hiding this comment

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

Unless I've missed something, it looks like this PR just duplicates functionality that it already present in the code?

int nCasesDays = validationParams.nCasesDays;

unsigned int cases_rows = observations.cases.size();
unsigned int cases_rows = Utilities::checkAndGetSize(observations.cases, "observations.cases");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused by this. It seems that checkAndGetSize is just doing a subset of what the ImportConsistencyCheck function is doing a few lines down? (The former only checks that there are a non-zero number of rows in observations.cases; the latter checks that there are exactly the correct number of rows and columns).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current ordering will cause a memory issue if the number of rows is zero:

unsigned int cases_rows = observations.cases.size();
unsigned int cases_cols = observations.cases[0].size();

I agree my method might not be needed but if you plan to drop it then we need to reorder so that the rows are checked before the columns are retrieved, i.e.:

unsigned int cases_rows = observations.cases.size();
ImportConsistencyCheck(scot_data_file, cases_rows, (nHealthBoards + 1), "rows");
unsigned int cases_cols = observations.cases[0].size();
ImportConsistencyCheck(scot_deaths_file, deaths_cols, nCasesDays, "columns");

instead.

observations.waifw_norm.size(), commonParameters.paramlist
);

Utilities::checkAndGetSize(observations.pf_pop, "observations.pf_pop");
Copy link
Contributor

Choose a reason for hiding this comment

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

Notwithstanding my comment above, I'd suggest that once you have checked that the data are valid once in the IO code, there's no reason to check again in the model code.

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.

3 participants