-
Notifications
You must be signed in to change notification settings - Fork 34
Fix infection origin: remove unnecessary UC check #407
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
Conversation
5d29b5b
to
d9c9441
Compare
d9c9441
to
2bfe526
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've given this a brief review and I can see that this change makes the code consistent with the base class' method documentation 👍
openmalaria/model/Clinical/CMDecisionTree.h
Lines 69 to 78 in b31d36a
/** Create a user-configured decision from an XML node. | |
* | |
* Memory management is handled internally (statically). | |
* | |
* @param node XML element describing the tree | |
* @param isUC If isUC is false and a "case type" decision is created, an | |
* xml_scenario_error exception is thrown. CMHostData::pgState is only | |
* used by the "case type" decision, so if isUC is false when creating the | |
* tree, pgState does not need to be set when executing the tree. */ | |
static const CMDecisionTree& create( const ::scnXml::DecisionTree& node, bool isUC ); |
I have some questions slightly beyond the scope of this PR.
I don't suppose you know why the case type decision tree isn't meant to work with complicated cases? I don't yet understand the full context surrounding this code.
I had a look at the schema to see if the docs there explained it. Interestingly, it seems like:
- The schema actually permits a case type decision tree with a complicated case.
- The exception being thrown in
CMDTCaseType::create
is where the rule against using a case type decision tree with a complicated case is enforced?
As far as I can tell, the reason that the schema permits using a case type decision tree with a complicated case is this line below.
openmalaria/schema/healthSystem.xsd
Line 494 in b31d36a
<xs:element name="complicated" type="om:DecisionTree"/> |
Maybe it is simpler to enforce the rule in CMDTCaseType::create
than in the schema..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Happy for this PR to be merged regardless of what the answers to the above questions may be.)
Exactly, the DecisionTree is defined once in the xml (in a recursive manner) and the same definition is used in both the intervention section and the health system section. So it's easier to work with this one definition and enforce the extra rule in the C++ code. The CaseType is defined as: "A switch which chooses a branch deterministically, based on whether the patient was treated recently (second line) or not (first line). For uncomplicated cases only." This only works for health system treatments, not treatments given through interventions, which are a bit more flexible. |
Makes sense - thanks for clarifying. |
Fix related to: #402
The UC check is not necessary for InfectionOrigin. This can be used both inside an intervention and for Uncomplicated Care in the health system.