1012 bug in getting testparameters in specific test derived from generictest

[khoanguyen-dev]

Changes and Information

Please briefly list the changes (main added features, changed items, or corrected bugs) made:

• Change TestType to enum class
• Create TestData struct to get TestParameter for each type
• Add a test to test the get_default()
• Delete the GenericTest and other type
• Modify the previous tests to accommodate these changes

If need be, add additional information and what the reviewer should look out for in particular:

• The changes are mostly made in parameters.h
• The TestData is added as one of the World parameters and TestParameters are extracted when they are needed.

Merge Request - Guideline Checklist

Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.

Checks by code author

• Every addressed issue is linked (use the "Closes #ISSUE" keyword below)
• New code adheres to coding guidelines
• No large data files have been added (files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
• Tests are added for new functionality and a local test run was successful (with and without OpenMP)
• Appropriate documentation for new functionality has been added (Doxygen in the code and Markdown files if necessary)
• Proper attention to licenses, especially no new third-party software with conflicting license has been added
• (For ABM development) Checked benchmark results and ran and posted a local test above from before and after development to ensure performance is monitored.

Checks by code reviewer(s)

• Corresponding issue(s) is/are linked and addressed
• Code is clean of development artifacts (no deactivated or commented code lines, no debugging printouts, etc.)
• Appropriate unit tests have been added, CI passes, code coverage and performance is acceptable (did not decrease)
• No large data files added in the whole history of commits(files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
• On merge, add 2-5 lines with the changes (main added features, changed items, or corrected bugs) to the merge-commit-message. This can be taken from the briefly-list-the-changes above (best case) or the separate commit messages (worst case).

Closes #1012

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.08%. Comparing base (43aec13) to head (a02e583).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1013      +/-   ##
==========================================
- Coverage   96.09%   96.08%   -0.01%     
==========================================
  Files         131      131              
  Lines       11046    11048       +2     
==========================================
+ Hits        10615    10616       +1     
- Misses        431      432       +1     

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

[DavidKerkmann]

We already talked about these changes and I am fine with it. We can discuss about the naming "TestData", so if anyone finds a better name, feel free to suggest it.

[reneSchm]

Good fix, note that the type mio::CustomIndexArray<mio::abm::TestParameters, mio::abm::TestType> needs to be bound for the py simulation to run.

[khoanguyen-dev]

There is still an error in the Pycode. The mio::CustomIndexArray<mio::abm::TestParameters, mio::abm::TestType> could not be bound since the pymio::bind_CustomIndexArray function does not include an overload for a tuple of doubles (just a single double).
I will try to work to extend this. If time is essential, I can set up the TestParameters in the test directly instead of taking it from the world.parameters.

[reneSchm]

According to CodeCov the coverage dropped because of some "indirect changes" by commit a8dd034 (I am not sure that the branch is reported correctly), so now line 200-202 in cpp/models/abm/infection.cpp are no longer covered. Probably some parameter change in the tests could fix this, but maybe have a look at the function as well (it might possibly need updating?).

[DavidKerkmann]

It seems that I managed to fix the python bindings. However, I had to implement serialise and deserialise for the simple

struct TestParameters {
     UncertainValue<> sensitivity;
     UncertainValue<> specificity;
}

as it couldn't be detected by the templates in io/io.h. Is there a way around that? I can imagine letting this derive from std::pair and write access operators for .first and .second but there may be something more simple. Please comment if you have ideas.

[reneSchm]

Is there a way around that?

Not really, we cannot iterate over members of generic structs. (Technically, we could using a void*, but that will cause more issues than it solves.)

I can imagine letting this derive from std::pair and write access operators for .first and .second

That (or tuples, vectors, or other types with its own (de)serialize_internal function) would work, but you lose the names when writing the Json.

I'd say the best options are

• Write the (de)serialize functions. It is not too much work, and does everything you need.
  • I am also working on a shorter serialization feature in the branch connected to branch 652, see here for example.
• Replace the TestParameter completely by a std::pair. You can use std::tie or structured binding to set the names, e.g.

auto& [sensitivity, specificity] = params.get<GenericTest>();

[DavidKerkmann]

I see. In that case I would leave it as is for now and make changes later if we need to.

[reneSchm]

Seems like everything works now, good fix for the CustomIndexArray bindings!
Before you merge, could you check the benchmarks? Though I expect there won't be any changes.

[DavidKerkmann]

Benchmark comparison. There seems to be some difference in timing for each run, but the values don't vary much from the main branch.

This branch:

Run on (8 X 2300 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 6144 KiB (x1)
Load Average: 6.39, 4.85, 4.04
---------------------------------------------------------------------------
Benchmark                                 Time             CPU   Iterations
---------------------------------------------------------------------------
abm_benchmark/abm_benchmark_50k        1396 ms         1308 ms            1
abm_benchmark/abm_benchmark_100k       2906 ms         2626 ms            1
abm_benchmark/abm_benchmark_200k       6221 ms         5678 ms            1

Main:

Run on (8 X 2300 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 6144 KiB (x1)
Load Average: 4.09, 4.75, 4.24
---------------------------------------------------------------------------
Benchmark                                 Time             CPU   Iterations
---------------------------------------------------------------------------
abm_benchmark/abm_benchmark_50k        2055 ms         1635 ms            1
abm_benchmark/abm_benchmark_100k       3202 ms         2823 ms            1
abm_benchmark/abm_benchmark_200k       7579 ms         6475 ms            1