ASSERTION in openehr_expression_104.bmm is not in accordance with the specifications

pieterbos · 4 January 2022 15:22

Pull request to do the rules changes in Fix json rules by pieterbos · Pull Request #377 · openEHR/archie · GitHub . Mostly configurable to be able to upgrade archie without too much trouble, and parsing is backwards compatible for easier upgrading.

Most work there is the configuration and the mechanism, not the renaming. I made it a draft pull request. So, @thomas.beale , are the names in Fix json rules by pieterbos · Pull Request #377 · openEHR/archie · GitHub as they should be?

I will add tests of course. After this is done, next week we hope to merge all this and be able to release a major archie release soon. Since it’s not fully compatible with Archie 1, will likely be 2.0.0 (semver).

thomas.beale · 4 January 2022 15:32

The changes look right to me - I wonder if you are making things too configurable however? The Archie code will break for sure but I suspect your (i.e. Nedap) system is the only user of that code so far - EhrBase doesn’t have any rule logic in its templates as far as I am aware - so dealing with the breakages locally is pretty trivial.

You could arguably consider this change a patch-level change to Archie if we agreed consider the missing name prefixes an ‘error’ - not casting aspersions within this group, I’m just thinking about how it looks from the outside / under semver logic.

pieterbos · 4 January 2022 15:46

There are more changes that break things, also to the API of Archie, so it’s a major release.

No, not too configurable. We have android apps parsing these rules with Archie, and we need to be able to update both our server and our apps without waiting for all apps to be updated. This means we need a proper migration path where this configurability is absolutely necessary.

thomas.beale · 4 January 2022 15:50

Good point - what is in the JSON matters, and if you don’t want to be chained to doing some sort of magic whole-of-system instant update, then you have to allow both.

I do wonder though if the config settings can be dispensed with in a later ?minor version - to reduce complexity a bit, and also not to give new Archie users the idea that there really is an intentional choice available here?

pieterbos · 4 January 2022 16:09

Yes, some of these config settings (not all!) can be removed in a later version. I could mark the configuration settings as deprecated right from the start. But actually removing them will be a major version no matter what, even if they are deprecated, since it changes the behaviour considerably and will break apps if it is still used.

NeoEHR · 4 January 2022 17:58

Great update Pieter!

@thomas.beale

EXPR_CONSTANT
EXPR_CONSTRAINT
EXPR_ARCHETYPE_ID_CONSTRAINT
EXPR_ARCHETYPE_REF

are missing in “openEHR_am_206.bmm” (and 210, 220, 230).
AOM 2.0.6

DavidIngram · 4 January 2022 18:33

Pieter, Borat, Pablo, Thomas, Sebastien, …
I e-mailed Thomas, yesterday, but should maybe say it here, too. I’m so impressed with your recent combined and coordinated effort. Thank you. A veritable trio/quartet/quintet/ … of string musicians tuning up - music to my ears in the orchestration of openEHR! David

pieterbos · 5 January 2022 13:38

Thanks for your kind words David!

I did some more changes:

Archie used RULE_SECTION instead of LIST in json. Has been fixed, of course it parses both formats for easy migration.
the op_ prefix in OperatorKind is also configurable now in serialization, again it parses both formats.
renamed RULE_STATEMENT to STATEMENT in json (probably never used directly?)
added some more tests

Review with co-worker should happen next week, but feel free to add a review to the github pull request.

thomas.beale · 5 January 2022 17:20

Is RULE_SECTION something like RULE_GROUP, conceptually in your implem?

pieterbos · 5 January 2022 19:55

No, just a representation of the entire rules section of the archetype. I have no idea why I introduced that part.

NeoEHR · 8 January 2022 07:57

This thread started with a question for ASSERTION in “openehr_expression_104.bmm”.

Many other things were achieved through the discussion - an updated version of Archie, small fixes to BMMs, an agreement to work out a solution for the EL spec.

I also got the ASSERTION (and EXPR_… and OPT2 JSON converter) working. Here is how a generated OPT2 model class for an ARCHETYPE_SLOT looks using the ASSERTION constraint:

…and here is a COMPOSITION class (Vital signs OPT):

Thank you Pieter and Thomas for helping me figure this out.

p.s.
@pieterbos While generating constraints for the attributes I noticed our OPT2 JSON differ for C_ATTRIBUTE_TUPLEs. After your JSON update lands in the VSCode extension I’ll use your JSON to test if it is only visually different from mine or are there also structural differences to what the AM’s OPERATIONAL_TEMPLATE expects.

Tuples (and constraints in general) take many lines to express in the JSON format. Here is an example of a generated tuple constraint for temperature (much shorter ):

…and for height in cm or inches:

…and for blood pressure (no tuples):

thomas.beale · 16 February 2022 18:18

pieterbos:

Modifications I had to make to make this work:

added a FOR_ALL_STATEMENT class, because it does not fit in BINARY_OPERATOR (missing the variable declaration), see archie/ForAllStatement.java at master · openEHR/archie · GitHub

ASSERTION does not have a variables attribute. Does not make sense to have, and the class wasn’t defined in the spec back then.

EXPR_ARCHETYPE_ID_CONSTRAINT is implemented, but never used, since at grammar level, this cannot be easily instantiated - a normal Constraint with a C_STRING is used in these places.

MODEL_REF has a variableReferencePrefix attribute in Archie, to indicate the path starts with a variable (as used in for_all)

OPERATOR_KIND has op_module added

added FUNCTION to implement the function calls specified in the language spec, which was missing in the object model, with attributes:

I’m just getting back to looking at this again. I see there are changes on branches in Archie, but am not sure of where you got to finally on this. Any chance you could make a new bullet point list of differences between the 1.0.4 spec (as it is now) and what you have in the code (today) - I assume this will be a shorter list than before. Then I’ll know what to concretely fix on the spec side.

thomas.beale · 14 March 2022 13:31

The way I have done this in the new EL grammar is to treat for_all and there_exists as Boolean leaf objects rather than binary operator, i.e.:

//
// Atomic Boolean-valued expression elements
//
booleanLeaf:
      booleanValue
    | forAllExpr
    | thereExistsExpr
    | '(' booleanExpr ')'
    | constraintExpr
    | SYM_DEFINED '(' valueRef ')'
    | arithmeticComparisonExpr
    | objectComparisonExpr
    | valueRef
    ;

//
//  Universal and existential quantifier
//
forAllExpr: SYM_FOR_ALL localVariableId ':' valueRef '|' booleanExpr ;

thereExistsExpr: SYM_THERE_EXISTS localVariableId ':' valueRef '|' booleanExpr ;

In the next generation object model (BMM) they don’t need matching objects, because for_all is really just syntax for calling a function for_all (lambda(v:T) {expr with v}: Boolean) on a Container type (List, Set etc). I.e. it resolves to a function call that is assumed to exist by the expression compiler.

In the old EL that we have in ADL/AOM2 there is no support for lambdas as such so it might be easier to support for_all and there_exists with model classes that will enable these expressions to be evaluated independently from container types.

thomas.beale · 14 March 2022 15:17

Another cosmetic change we should make is that EXPR_FUNCTION should have been called EXPR_FUNCTION_CALL, which is what it really stands for, and as far as I can see from the Archie code, how it is understood.

pieterbos · 14 March 2022 15:34

Archie with the mentioned JSON changes in this thread was released last week.

thomas.beale · 14 March 2022 15:35

I think this must be some intermediate form of model - I couldn’t find this in the currently published version, and it is not in the UML.

thomas.beale · 14 March 2022 15:54

I had a look at the Archie code - I have modified the rules part of the AOM a bit (the inheritance was wrong, for one thing), but I might leave this class in, if we can consider the Archie implementation to be a reasonable interpretation / simplification.

thomas.beale · 14 March 2022 16:31

I couldn’t find this, but I simplified the Operation definitions part of the model. Archie has an enum that I can’t easily replicate in UML (Java’s enums are more sophisticated than UML’s…)

thomas.beale · 5 April 2022 14:44

I have now added a for_all operator to the model, in the same place as you have it in Archie. Naming slightly different to be a a bit clearer, but since there are already other minor naming differences, I don’t think this makes any difference.

Note: you don’t have a there_exists operator in Archie, so I didn’t add one, but it would be easy if needed.

I believe this version of the model is a pretty good match to the Archie implementation.

EDIT: sorry, meant to link it → see here.

sebastian.iancu · 20 July 2023 10:45

A bit related to original posts, and to follow up above (although it is hard to tell exactly …):

I recently notice that the ASSERTION class definition is gone from specs (only the BMM_ASSERTION remained). The AOM 2.0.6 UML still refers to it (see diagram for P_ARCHETYPE_SLOT at Archetype Object Model 2 (AOM2)) but in the latest is not (see same P_ARCHETYPE_SLOT' on diagram at Archetype Object Model 2 (AOM2)).

I suspect this UML issue is related to all discussion above - I just wonder what would be the fix, and where is the ASSERTION class nowadays.

For reference: