All OpenMRS ID accounts have been reset.

Read more and change your password before signing in.
Icon

EXTENDED: OMRS14 Proposals due 30 April! Read more and submit a proposal at OpenMRS Talk.

Skip to end of metadata
Go to start of metadata

High-level plans

New functionality

A new "rule" module will be authored which has the core set of functionality that is desired. This functionality will be primarily focused on producing a particular piece of data for a single patient or a cohort of patients. This module will use the "org.openmrs" namespace, rather than the conventional "org.openmrs.module" namespace in order to more easily support the goal of eventually incorporating this module into core. By keeping it as a module initially, this allows implementations running older versions of OpenMRS (1.6, 1.8, 1.9, etc) to take advantage of, and test out, this work, without requiring them to upgrade. It also will allow the module to evolve at a different pace than the core code.

TODO: What do we think of the name of this module? rule? patientcalculation? patientdata?

For the purposes of this design, and for brevity, I will use "Rule". If we change the name, we can replace "Rule" throughout as appropriate.

Existing functionality

The existing "org.openmrs.logic" package that exists in core will be removed into the existing logic module. The logic module will be updated such that it requires the module described above, and that it implements the interfaces created and exposed in it. The logic module will no longer be a "core" or "required" module within an OpenMRS distribution. Backwards compatibility will be maintained with a few minor exceptions, including the need to explicitly require the module within any module that uses it, and the need to use Context.getService(LogicService.class) rather than Context.getLogicService().

Requirements for the new functionality

Design for new functionality

Rule / ParameterDefinition

A Rule represents a definition that can be evaluated to produce patient data. A Rule can expose parameters which control the results of it's calculation.

RuleContext

The RuleContext contains any contextual information that may be shared across one or more Rule evaluations. This includes the "index date" for the evaluation and a cache for storing the results for previously evaluated rules. The index date represents the date on which the evaluation should occur. It should essentially replace any call for "new Date()" in evaluation code, and should return the data that was accurate as of that particular date and time.

Result

A Result is the data that is produced from evaluating a Rule for a single patient. Results are strongly typed, but provide a convenience method for casting to other datatypes.

We will likely employ a library of utility methods as well, to support conversion of Result types. For example:

CohortResult

A CohortResult is the data that is produced from evaluating a Rule for a Cohort of patients. It is essentially a wrapper of a Map<Integer, Result>, but provides the flexibility to add additional methods and/or data as needed down the road.

RuleEvaluator

A RuleEvaluator is responsible for evaluating one or more types of Rules into Results. This is where the bulk of all calculations occur, either by performing these calculations directly within the evaluator, or by calling service methods / DAOs that perform calculations. RuleEvaluators will likely be wired to Rule classes either via a registry or via annotations.

As an implementation detail, we probably want an abstract class to simplify writing RuleEvaluators that evaluate patients one at a time, like:

RuleProvider

A RuleProvider is responsible for retrieving a Rule instance given a rule name and an optional configuration string. A typical implementation would be such that ruleName is the rule class to instantiate and configuration represents the serialized property values that need to be configured on this rule instance, however it is totally up to the Provider to define this. For example, to retrieve the Rule for "Most Recent Weight":

Rule Name: org.openmrs.rule.definition.MostRecentObsRule
Configuration: concept=<UUID for Weight (KG) concept>

There would then be a RuleProvider registered to handle this type of Rule which would know it needed to first instantiate a new instance of MostRecentObsRule, configure it's properties via the parsed values from the configuration string, and then return configured Rule instance. Like RuleEvaluators, RuleProviders will likely be wired to Rule classes either via a registry or via annotations.

TokenRegistration

A TokenRegistration represents a saved Rule instance in the database, and includes a unique name, the RuleProvider, the ruleName, and the configuration for the rule. The intention is to allow a fully configured Rule instance to be retrieved given a unique name String. This class is a hibernate-managed class.

RuleService

The RuleService is the primary mechanism for evaluating Rules and for associating Rule instances with saved tokens.

50 Comments

  1. Looks good, this is what we've been iterating towards.

    A few assorted comments:

    • I like PatientCalculation or Calculation, as a name for this new thing--I think that's pretty intuitive for both developers and users, and describes what this thing is. I know I originally pushed for "Rule," but thinking about it more, Rule implies some sort of action, and these are just calculations.
    • We should consider having parameters use the new CustomDatatype and Handler interfaces, rather than just use Class. We'd need to find a way to do this without requiring OpenMRS 1.9 though.
    • DONE Consider having CohortResult implement Map<Integer, Result>
    • DONE shouldn't Result and CohortResult have a pointer to the context they were evaluated in?
    • DONE I'm not sure we should make having a cache on RuleContext be optional. I think we have to promise some specific behavior, for RuleProviders to code against.
    • DONE shouldn't the method sig be: void deleteToken(Token)?
    1. I have updated the design above to reflect the latter 4 points. We can discuss those in-context, and have a separate discussion on the first 2 points.

  2. Is the RuleService going to cache any rule classes and return a new instance of the class when a Rule is requested?  The current LogicService implementation now does this (except for the new instance part), and it prevents unneccessary database hits.  The problem before was the logic_token_registration table was accessed each time a rule was requested.  This was a performance hit for the CHICA team because we were constantly requesting rules due to the high number of them we execute.

    1. I agree that caching token registrations is important, otherwise we'll get a significant performance hit. Would this change any of the method signatures in RuleService though? Currently we just have "Rule getRule(String tokenName)", and I think that works fine, regardless of whether we're caching the token registrations.

      1. It would not change any of method signatures.  I just wanted to throw it out there as an implementation detail for when the time comes to do the coding.

  3. @Darius - would you be able to update the above design based on the discussions on today's design call (and potentially also your notes above)?

  4. Rule
    • Having key + name + description in parameter definitions is weird. Parameters are usually defined as a Hash, aren't they? Also, no need to call the method "getParameterDefinitions()" in the Rule interface, since it's clear that when querying a Rule definition for parameters, you're not going to get parameter values
    • How does a rule expose a result (or any way for a RuleEvaluator to evaluate it)?
    RuleContext
    • Are we defining index date as a date (day) or a datetime (point in time)?
    • Are there any conventions for caching or are we exposing a simple caching mechanism per context where you can store an object under a unique string for the life of the context? How will rules avail themselves of the cache? Is this up to each implementation to figure out?
    RuleEvaluator
    • I'd prefer to have the context first in the evaluate() method... for, er, context. (smile)
      • Having key + name + description in a parameter definition doesn't seem at all weird to me. I think the Set<ParameterDefinition> return type Mike proposed is fine.
      • I guess it's an implementation choice how RuleService internally decides what RuleEvaluator is appropriate for a given Rule. I imagine Mike will propose doing this via our @Handler annotation. Personally I'm not sure I see a need to separate the RuleProvider and RuleEvaluator interfaces. (If they were merged, the a TokenRegistration tells you which class to use to instantiate a rule and to evaluate it.)
      • My first instinct is to make an index date a datetime (point in time).
      • I propose having the cache just be a very simple mechanism that lets you store String->Object in the context, and you have to just know how to fetch something with the name you stored it under. (I do think we should give a convention for cache keys...)
      • In RuleService, we have versions of the method both with and without RuleContext. Given that, I'd prefer to have it be the last parameter. (E.g. eval(Cohort, Rule) and eval(Cohort, Rule, RuleContext) makes more sense to me than eval(Cohort, Rule) and eval(RuleContext, Cohort, Rule). But that's a matter of preference, I guess. Wyclif, you can do whatever seems most natural.
  5. So from the description at the top, looks like the required OpenMRS version is 1.6, right?

    1. Correct, support for 1.6+ is required.

  6. I need some clarification about CohortResult, i assumed it is supposed to extend a standard map implementation like a linkedHashMap, if this is the case, i don't seem to know why it needs to have getAllResults(). On the other hand, if i'm supposed to write my own map implementation,  is this really worth the effort?

    1. FYI, for all occurences of Rule on this page, in my code an using Calculation, so i have classes and interfaces like CalculationContext, CalcluationProvider, CalculationEvaluator etc. Hope you are fine with this

      1. Can we come up with a longer word than Calculation? ;)

        1. We could follow the lead of Obs, and just call it Calc.

          

          Note

          Icon

          The previous sentence was a joke. We will call it Calculation. And Burke will learn how to do Command-Space.

          1. I feel sorry for the person who creates the first universal medical calculator module for OpenMRS. :p

            1. @Burke, it is a calculation module not a calculator module, (big grin)

        2. I'd be happy with "data" or "query" as the module id name, if those resonate better with people. For example "org.openmrs.data.patient..." or "org.openmrs.query.patient...". I like these better myself I think, but will defer to the group.

    2. Yes, that's what I expect.

    3. Right, it looks like the getAllResults() method was left-over from before it implemented Map<Integer, Result>. You don't need the getAllResults() method. Just have it extend HashMap<Integer, Result> and leave it otherwise empty for now. We can add additional convenience methods into this if and when needed.

  7. I guess CalculationEvaluator.evaluate(Cohort, Calculation, Map<String, Object>, RuleContext); doesn't need the map of parameter values argument since these can be fetched from the passed in calculation

    1. That's not true. the Map<String, Object> parameters are provided by the user at evaluation-time. The calculation only knows about the definitions of parameters. (FWIW, I disagree with Burke's comment about renaming Calculation.getParameterDefinitions() to Calculation.getParameters(), since I think it's better to be overly clear.)

      1. Yep, I agree with Darius. This confusion is exactly why we called them ParameterDefinition and not Parameter

  8. Is there any information that the framework automatically adds/updates in the calculation after the evaluation is done besides the index date, or that is left to the consuming code?

    1. The index date should not be set after the calculation is done. If the indexDate is null when service.eval is called, it should be set to now(), but index date tells the calculation evaluators what date to do their calculations against. It does not mean "when was the calculation evaluated".

    2. The index date represents "now" for all calculations within that context.  By convention, calculations should never use the current time; rather, they should use the current context's index date to represent the current time.  For example, an AgeCalculation should calculate age based on the time difference between date of birth and the index date instead of using new Date().

      Initially, we were using index date to represent the value of "today."  Since we've evolved this to be a datetime, we might want to replace index date with a CalcluationContext.now() method.

      1. I like that, it's a lot more clear. We should call it getNow(), so it can be accessed as a standard bean property.

        1. we currently have getters and setters for indexDate,  burke and darius you seem to imply that we should replace the getter with getNow() and maintain the setter

          1. No, let's get rid of "indexDate" completely, and use the name "now" both for the getter/setter, and for the private Date property.

            "Index Date" was always a bit confusing, and "now" is clearer.

  9. I guess there are calculations that we intend to have shipped with the module, what are the suggestions? I intend to work on these now that the core functionality is in place

    1. I don't think we should ship any calculations with the module. (Maybe just some very simple ones.) The real implementations of Calculation should be the reporting and logic modules.

      It's fine to write some calculations like this age one and last-obs-value for testing purposes, and to clarify the API, but please spend as little time as possible designing/coding/refactoring those calculations.

    2. Are we planning on shipping a Calculation implementation within the Calculation module?  I thought not.  Rather, I thought that we would retrofit the next release of the Logic module to implement calculations and, if we wanted to introduce a "new" approach to logic, it would be within a new Logic 2.0 or other module that implemented Calculation.  So, the Calculation module merely supplies the framework.

      1. I agree. We should not ship any Calculation implementations with this module. If we want to provide examples, they can go in the test package and double as test cases and examples for how modules can utilize the framework.

  10. Isn't the return type of ResultUtil.getFirst(Result result) supposed to be Object?Assuming the value of the result is a list as described above?

    1. It should go something like the following:

      (But with null-checking and deal with empty results.)

      1. ListResult is not a subclass of Result, it is  wrapper for a List i.e it extends ArrayList or LinkedList(i used ArrayList), unless Mike meant if the Result.getValue returned a collection.

        1. Look at the wiki page above. ListResult is supposed to implement Result. We're trying to get the power of Burke's original idea that "everything is a Result" without some of the downsides that came with it.

          1. Then it sounds like ListResult should have a backing list as its property, right?

  11. What is the behavior we expect from Calculation.getCalculation(String tokenName), this seems to be associated to the registered tokens, do we want to implement this?

    1. This is in CalculationService, not Calculation, right? Yes, this is about using registered tokens. Basically you'd fetch the TokenRegistration for the given tokenName, and then you'd use the appropriate provider to instantiate and configure a Calculation, then return that.

  12. Won't it be necessary to add Parameter definitions associated to domain objects that will be common parameter datatypes for calculations? Off the top of my head i think we might need ConceptParameterDefinition

    1. No, the datatype property of a parameter tells what class its value should be.

  13. I guess what i meant, was instead of someone always having to create their parameter definitions, we can provider a quicker way to do so, compare the snippets below:

    rather than:

    I added utility methods to CalculationUtil like:

    But the first example above would be quite useful for date and concept since these will the most widely used parameter data types

    1. Wyclif, ParameterDefinitions are supposed to be dumb objects, so we don't need to have a version for each parameter type, just a single BaseParameterDefinition. E.g. if I have a calculation that requires two dates and a location, I should be able to do something like:

      For convenience we could provide a utility class that supports the builder pattern, something like this:

  14. DELETE ME. (Moved to a reply to another comment.)

  15. Don't we want to have the framework provide a validation abstraction that can be invoked before evaluating the calculation? Currently the only supported one is for required parameter values

  16. It isn't critical to me that this is implemented as our first pass.

  17. We decided to rename classes/interfaces in the module and prefix them with Patient e.g. PatientCalculationService, PatientCalculationContext etc, seems to me like we should also be renaming Results to PatientResult since the result actually has patient specif properties i.e PatientCalculation and PatientCalculationContext.
    What do you think?

    And does it mean we are changing CalculationProvider.getCalculation to return PatientCalculations? and rename the method to getPatientCalculation()
    Wyclif