-
Notifications
You must be signed in to change notification settings - Fork 27
Support conversion of ADL1.4 to ADL2 at-coded #658
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
base: adl_2.4_support
Are you sure you want to change the base?
Conversation
| } | ||
| } | ||
|
|
||
| List<String> termsToRemove = new ArrayList<>(); |
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.
Old code assumed all nodeId's were converted, now this is not the case
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## adl_2.4_support #658 +/- ##
==================================================
Coverage 72.20% 72.20%
- Complexity 7121 7135 +14
==================================================
Files 672 672
Lines 23156 23195 +39
Branches 3755 3767 +12
==================================================
+ Hits 16719 16748 +29
- Misses 4697 4702 +5
- Partials 1740 1745 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aom/src/main/java/com/nedap/archie/adl14/ADL14ConversionConfiguration.java
Show resolved
Hide resolved
aom/src/main/java/com/nedap/archie/adl14/ADL14ConversionConfiguration.java
Outdated
Show resolved
Hide resolved
aom/src/main/java/com/nedap/archie/adl14/ADL14ConversionConfiguration.java
Outdated
Show resolved
Hide resolved
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.
Initial review done. I will test a bit more and also asked Joost if he can help with some testing. See also the comments of Joost (and my reaction to some of those)
aom/src/main/java/com/nedap/archie/adl14/ADL14ConversionConfiguration.java
Show resolved
Hide resolved
aom/src/main/java/com/nedap/archie/adl14/ADL14ConversionConfiguration.java
Outdated
Show resolved
Hide resolved
tools/src/test/java/com/nedap/archie/adl14/ADL14ToADL2Test.java
Outdated
Show resolved
Hide resolved
tools/src/test/java/com/nedap/archie/adl14/ADL14ToADL2Test.java
Outdated
Show resolved
Hide resolved
|
I see you marked all my remarks as done. It looks like you applied all my suggestions. Thank you. Is there anything else you'd like me to review? |
@joostholslag No that's fine, I just wanted to let you know I've implemented your comments and if you want you can look at it again. We are testing manually with a few more archetypes at our side |
|
Cool. Nicely done. Thank you for the work! |
gardes
left a comment
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.
Hi all,
I love this branch and the at-coded 2.4 conversion routine and could not resist to play with it - very nice work!
One thing I noticed is that the conversion of C_DV_ORDINALs from 1.4 archetypes to 2.4 at-coded archetypes may have a problem. It is best shown with an example from a CKM archetype, e.g. https://ckm.openehr.org/ckm/archetypes/1013.1.137 :
The converted at-codes are incremented by 1 and not zero-padded. I would have expected them to remain unchanged and zero-padded?
Without this, it is well possible that the terminology has both of at14 and at0014 in the terminology, but with completely different meanings. There are actually examples in other archetypes where this happens in practice, see at27 vs at0027 - example from: https://ckm.openehr.org/ckm/archetypes/1013.1.7006/adl :

Order of terminology codes
Also the order of terminology codes is different than in the example archetype, because the order is dependent on when the objects are added to the map. Is this a problem?
It would be useful if they are in the natural alphabetical order of the codes, this is what we typically do I think (and may be easier to implement than explicitly keeping the original order in 1.4. In most cases, the result will be identical.
Demo archetype
BTW, I often like to use the Demonstration archetype in CKM https://ckm.openehr.org/ckm/archetypes/1013.1.1385 for any testing purposes, maybe it is helpful for you.
I hope these comments are helpful to you! Kind regards,
Sebastian


In this PR, a configuration variable is added to the ADL14ConversionConfiguration class to be able to convert adl1.4 archetypes into adl2 archetypes, but based on the at coded system, this will be supported from adl 2.4+.
Still to do: