SO-5535: MRCM evaluations #1068

AAAlinaaa · 2022-10-12T13:19:43Z

Related ticket: https://snowowl.atlassian.net/browse/SO-5501

... members with no generated relationships

codecov-commenter · 2022-10-19T11:48:56Z

Codecov Report

Base: 64.23% // Head: 64.26% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (ab252c7) compared to base (830f966).
Patch coverage: 74.73% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##                8.x    #1068      +/-   ##
============================================
+ Coverage     64.23%   64.26%   +0.02%     
- Complexity    12467    12516      +49     
============================================
  Files          1734     1741       +7     
  Lines         58217    58451     +234     
  Branches       5372     5394      +22     
============================================
+ Hits          37396    37562     +166     
- Misses        18506    18567      +61     
- Partials       2315     2322       +7

Impacted Files	Coverage Δ
...ational/snowowl/snomed/common/SnomedConstants.java	`100.00% <ø> (ø)`
...nowowl/snomed/core/rest/SnomedMrcmRestService.java	`4.76% <4.76%> (ø)`
...ational/snowowl/snomed/core/MrcmAttributeType.java	`50.00% <50.00%> (ø)`
...nomed/datastore/request/SnomedMrcmTypeRequest.java	`75.00% <75.00%> (ø)`
...tastore/request/SnomedMrcmRangeRequestBuilder.java	`80.00% <80.00%> (ø)`
...omed/datastore/request/SnomedMrcmRangeRequest.java	`92.30% <92.30%> (ø)`
...atastore/request/SnomedMrcmTypeRequestBuilder.java	`100.00% <100.00%> (ø)`
...owowl/snomed/datastore/request/SnomedRequests.java	`92.10% <100.00%> (+0.43%)`	⬆️
...n/detail/SnomedValidationIssueDetailExtension.java	`75.54% <100.00%> (ø)`
...x/src/com/b2international/index/util/JsonDiff.java	`75.29% <0.00%> (-2.36%)`	⬇️
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

...omed.core.rest.tests/src/com/b2international/snowowl/snomed/core/request/SnomedMrcmTest.java

cmark · 2022-10-26T07:58:10Z

...nomed.datastore/src/com/b2international/snowowl/snomed/datastore/request/SnomedRequests.java

@@ -386,5 +397,97 @@ public static SnomedRepositoryCommitRequestBuilder prepareCommit() {
 	public static SnomedConceptSearchRequestBuilder prepareGetSynonyms() {
 		return prepareSearchConcept().all().filterByActive(true).filterByEcl("<<"+Concepts.SYNONYM);
 	}
-
+
+	public static Promise<Collection<String>> getApplicableTypes(final IEventBus bus, final String resourcePath, 


Let's push this completely to the server side as one request to form a new Java API.

cmark · 2022-10-26T08:00:35Z

...nomed.datastore/src/com/b2international/snowowl/snomed/datastore/request/SnomedRequests.java

+					requestBuilder.filterByAncestor(SnomedConstants.Concepts.CONCEPT_MODEL_DATA_ATTRIBUTE);
+				} else if (!needsDataAttributes && needsObjectAttributes) {
+					requestBuilder.filterByAncestor(SnomedConstants.Concepts.CONCEPT_MODEL_OBJECT_ATTRIBUTE);
+				}


Add another else block with a meaningful comment for the missing case (both boolean parameters are false).

No longer applicable.

cmark · 2022-10-26T08:04:18Z

...nomed.datastore/src/com/b2international/snowowl/snomed/datastore/request/SnomedRequests.java

+					.execute(bus);
+			}).thenWith(members -> {
+				Set<String> typeIds = members.stream().map(m -> m.getReferencedComponentId()).collect(Collectors.toSet());
+				if ((needsDataAttributes && needsObjectAttributes) || typeIds.isEmpty()) {


What happens if I specify both boolean parameters with the value false?

Changed this to use an enum instead, so this wouldn't happen.

cmark · 2022-10-26T08:16:05Z

...tional.snowowl.validation.snomed/src/main/resources/scripts/rule_mrcm_constraint_type.groovy

+}
+
+def searchRelationshipsInUnregulatedDomains =  {
+	String regulatedDomainSpace = Joiner.on(" OR ")


If this is an ECL OR operator, then use Ecl.or helper instead.

cmark · 2022-10-26T08:16:33Z

...tional.snowowl.validation.snomed/src/main/resources/scripts/rule_mrcm_constraint_type.groovy

+	String regulatedDomainSpace = Joiner.on(" OR ")
+		.join(FluentIterable.from(domainMembers.values())
+			.transform({ m -> m.getProperties().get(SnomedRf2Headers.FIELD_MRCM_DOMAIN_CONSTRAINT)})
+			.transform({ c -> "(${c})"}));


This becomes unnecessary if you use that Ecl.or helper I think.

cmark · 2022-10-26T08:16:47Z

...tional.snowowl.validation.snomed/src/main/resources/scripts/rule_mrcm_constraint_type.groovy

+			.transform({ m -> m.getProperties().get(SnomedRf2Headers.FIELD_MRCM_DOMAIN_CONSTRAINT)})
+			.transform({ c -> "(${c})"}));
+
+	Set<String> unregulatedDomainSpace = getApplicableConcepts("* MINUS (${regulatedDomainSpace})");


Use Ecl.exclude helper.

cmark · 2022-10-26T08:17:29Z

...tional.snowowl.validation.snomed/src/main/resources/scripts/rule_mrcm_constraint_type.groovy

+	//Find non-IsA relationships in domains where no MRCM rule is defined
+	ExpressionBuilder relationshipQueryBuilder = Expressions.builder()
+			.filter(SnomedRelationshipIndexEntry.Expressions.active())
+			.filter(SnomedRelationshipIndexEntry.Expressions.sourceIds(unregulatedDomainSpace))


Is regulated/unregulated the same as sanctioned/unsanctioned? If yes, please rename these variables to the latter.

They might, I was trying to make a distinction between cases that break an explicit rule, vs cases that aren't covered by any rules. To me, an attribute that breaks an explicit rule is an unsanctioned attribute, attributes in domains that have no rules defined for them are unregulated.

We do offer both sanctioned and unsanctioned properties for selection.

cmark · 2022-10-26T08:18:52Z

...d/src/test/java/com/b2international/snowowl/validation/snomed/GenericValidationRuleTest.java

 				.domainConstraint(String.format("<<%s", Concepts.CONCEPT_MODEL_ATTRIBUTE))
 				.build();

 		final SnomedRefSetMemberIndexEntry mrcmAttributeDomainMember1 = member(Concepts.FINDING_SITE, Concepts.REFSET_MRCM_ATTRIBUTE_DOMAIN_INTERNATIONAL)
 				.referenceSetType(SnomedRefSetType.MRCM_ATTRIBUTE_DOMAIN)
+				.refsetId(Concepts.REFSET_MRCM_ATTRIBUTE_DOMAIN_INTERNATIONAL)


Do we have test cases for failing/passing OWL Axiom MRCM relationships?

...omed.core.rest.tests/src/com/b2international/snowowl/snomed/core/request/SnomedMrcmTest.java

...snomed.core.rest/src/com/b2international/snowowl/snomed/core/rest/SnomedMrcmRestService.java

...med.datastore/src/com/b2international/snowowl/snomed/datastore/request/MrcmRangeRequest.java

...omed.datastore/src/com/b2international/snowowl/snomed/datastore/request/MrcmTypeRequest.java

cmark · 2022-11-24T10:18:59Z

...omed.datastore/src/com/b2international/snowowl/snomed/datastore/request/MrcmTypeRequest.java

+				.setFields(ID, MRCM_RULE_REFSET_ID)
+				.build()
+				.execute(context)
+				.stream()


Please don't use all() anymore unless you have a really good reason to do so. Also, using all + stream together is a no go. Please set the limit correctly.

This request queries a small set of module scope members, which should be kept in practice at a low member count (the current tally is 3, and the expected count is 3 × number of modules declaring MRCM rules). stream() is called on the returned collection resource and processes individual members, not batches.

...omed.datastore/src/com/b2international/snowowl/snomed/datastore/request/MrcmTypeRequest.java

...med.datastore/src/com/b2international/snowowl/snomed/datastore/request/MrcmRangeRequest.java

...omed.datastore/src/com/b2international/snowowl/snomed/datastore/request/MrcmTypeRequest.java

...ternational.snowowl.validation.snomed/src/main/resources/scripts/rule_mrcm_constraint.groovy

...tional.snowowl.validation.snomed/src/main/resources/scripts/rule_mrcm_constraint_type.groovy

+	//Find non-IsA relationships in domains where no MRCM rule is defined
+	ExpressionBuilder relationshipQueryBuilder = Expressions.builder()
+			.filter(SnomedRelationshipIndexEntry.Expressions.active())
+			.filter(SnomedRelationshipIndexEntry.Expressions.sourceIds(unregulatedDomainSpace))


...ternational.snowowl.validation.snomed/src/main/resources/scripts/rule_mrcm_constraint.groovy

...attributes are needed

apeteri

One small observation below. I think the rest of my requests were addressed, so LGTM otherwise!

...snomed.core.rest/src/com/b2international/snowowl/snomed/core/rest/SnomedMrcmRestService.java

...atastore/src/com/b2international/snowowl/snomed/datastore/request/SnomedMrcmTypeRequest.java

cmark

🥓

AAAlinaaa added 6 commits October 4, 2022 17:36

[mrcm]: Support evaluation of applicable type/predicates

46def53

[mrcm]: Take module scopes into account in finding applicable predicates

d6d8095

fix(validation): Fix component type comparisons

99486b8

fix(mrcm): Limit results, by modules, attribute types

b661e21

fix(mrcm): Limit results by module scope, skip validation of axiom ...

7dda5a7

... members with no generated relationships

fix(mrcm): Use module scope in evaluation, skip concrete value ranges

75edb27

AAAlinaaa self-assigned this Oct 12, 2022

AAAlinaaa changed the title ~~Issue/so 5535 mrcm applicable predicates~~ SO-5535 MRCM evaluations Oct 12, 2022

AAAlinaaa added 3 commits October 12, 2022 18:27

fix(mrcm): Report issues in domains outside of MRCM domain ranges

3794b07

fix(mrcm): Remove debug messages

02c378d

fix(mrcm): Fix condition for skipping concrete value type ranges

304c669

AAAlinaaa mentioned this pull request Oct 14, 2022

[mrcm]: Support evaluation of applicable type/predicates #1067

Closed

AAAlinaaa marked this pull request as ready for review October 15, 2022 12:50

AAAlinaaa requested a review from cmark October 15, 2022 12:50

AAAlinaaa added the enhancement label Oct 15, 2022

AAAlinaaa added 5 commits October 18, 2022 11:41

fix(validation): Update validation tests launch configurations

cf1e25a

fix(tests): Ensure MRCM validation tests take refset scope into account

edc4bcd

test(mrcm): Add MRCM requests tests

66139ce

fix(mrcm): Handle duplicate entries for the same type

b2fb5ca

fix(mrcm): Add constant for precoordinated SCT content type

d4c2a40

cmark requested changes Oct 26, 2022

View reviewed changes

AAAlinaaa added 8 commits October 26, 2022 16:27

fix(mrcm): Add missing license header

aa29485

fix(mrcm): Add missing since tag

4c94712

fix(mrcm): Explain handling missing case of flag setting combinations

02cc2a2

fix(mrcm): Restrict member fetches to only the most necessary fields

f8f101b

fix(mrcm): Take self ids into account in calculating applicable rules

306b26c

fix(mrcm): Move concrete value type range prefixes to constants

7ea273d

fix(mrcm): Move markers of relationshipless axioms to a constant

95239e0

fix(mrcm): Use unsupported axiom type markers constant

57615e0

AAAlinaaa added 3 commits October 27, 2022 11:30

fix(mrcm): Use helper methods to construct ECL expression

c15371a

fix(mrcm): Create MRCM REST service

032f87d

fix(mrcm): Refactor MRCM requests

a11bcc1

cmark requested a review from apeteri November 13, 2022 14:55

Merge branch '8.x' into issue/SO-5535-MRCM_applicable_predicates

c4abc6d

cmark changed the title ~~SO-5535 MRCM evaluations~~ SO-5535: MRCM evaluations Nov 24, 2022

cmark requested changes Nov 24, 2022

View reviewed changes

apeteri requested changes Nov 24, 2022

View reviewed changes

apeteri reviewed Nov 28, 2022

View reviewed changes

...ternational.snowowl.validation.snomed/src/main/resources/scripts/rule_mrcm_constraint.groovy Outdated Show resolved Hide resolved

AAAlinaaa added 12 commits November 28, 2022 15:45

fix(mrcm): Use predefined constant

9a8522a

fix(mrcm): Make variable final

43c655c

fix(mrcm): Correct logical operator in case both object and data ...

5a8ff18

...attributes are needed

fix(mrcm): Add line breaks after switch cases

0123652

fix(mrcm): update since tags

ae68384

fix(mrcm): Rename MRCM requests

67286e8

fix(mrcm): Rename enum

3e2cc8e

fix(mrcm): Make visibility of new requests package private

2525acc

fix(mrcm): Fix request visibility

ba2eb0f

fix(mrcm): Move, rename attribute type enum

34d079e

fix(mrcm): Correct date in license header

031f412

fix(mrcm): Support paging, update endpoint names

ab252c7

apeteri approved these changes Dec 7, 2022

View reviewed changes

...snomed.core.rest/src/com/b2international/snowowl/snomed/core/rest/SnomedMrcmRestService.java Outdated Show resolved Hide resolved

...snomed.core.rest/src/com/b2international/snowowl/snomed/core/rest/SnomedMrcmRestService.java Outdated Show resolved Hide resolved

fix(mrcm): Update endpoint names

666fe2a

cmark requested changes Dec 7, 2022

View reviewed changes

...atastore/src/com/b2international/snowowl/snomed/datastore/request/SnomedMrcmTypeRequest.java Outdated Show resolved Hide resolved

fix(mrcm): Use predefined Ecl constant

d5914a9

cmark approved these changes Dec 7, 2022

View reviewed changes

cmark merged commit fea71e2 into 8.x Dec 7, 2022

cmark deleted the issue/SO-5535-MRCM_applicable_predicates branch December 7, 2022 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SO-5535: MRCM evaluations #1068

SO-5535: MRCM evaluations #1068

AAAlinaaa commented Oct 12, 2022 •

edited by cmark

Loading

codecov-commenter commented Oct 19, 2022 •

edited

Loading

cmark Oct 26, 2022

cmark Oct 26, 2022

AAAlinaaa Oct 28, 2022

cmark Oct 26, 2022

AAAlinaaa Oct 28, 2022

cmark Oct 26, 2022

cmark Oct 26, 2022

cmark Oct 26, 2022

cmark Oct 26, 2022

AAAlinaaa Oct 27, 2022

This comment was marked as duplicate.

AAAlinaaa Nov 28, 2022

cmark Oct 26, 2022

AAAlinaaa Oct 27, 2022

cmark Nov 24, 2022

apeteri Nov 24, 2022

This comment was marked as duplicate.

apeteri left a comment •

edited

Loading

cmark left a comment

SO-5535: MRCM evaluations #1068

SO-5535: MRCM evaluations #1068

Conversation

AAAlinaaa commented Oct 12, 2022 • edited by cmark Loading

codecov-commenter commented Oct 19, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

apeteri left a comment • edited Loading

Choose a reason for hiding this comment

cmark left a comment

Choose a reason for hiding this comment

AAAlinaaa commented Oct 12, 2022 •

edited by cmark

Loading

codecov-commenter commented Oct 19, 2022 •

edited

Loading

apeteri left a comment •

edited

Loading