Skip to content
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

Dimension relation validations #199

Merged
merged 16 commits into from
Sep 30, 2024
Merged

Dimension relation validations #199

merged 16 commits into from
Sep 30, 2024

Conversation

tpluscode
Copy link
Contributor

@tpluscode tpluscode commented Sep 18, 2024

The git graph got a little ot of hand: Squash merge when approved.

cc @kronmar

re https://gitlab.ldbar.ch/bafu/visualize/-/issues/668

Copy link

changeset-bot bot commented Sep 18, 2024

🦋 Changeset detected

Latest commit: 7cb9da8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
cube-link Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Base automatically changed from shacl-test to main September 19, 2024 08:33
Comment on lines 194 to 204
[
sh:path
(
meta:relatesTo
[ sh:inversePath sh:path ]
[ sh:inversePath sh:property ]
) ;
sh:minCount 1 ;
sh:class cube:Constraint ;
sh:message "value of meta:relatesTo must be a cube dimension" ;
] ;
Copy link
Contributor Author

@tpluscode tpluscode Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With SHACL-SPARQL we would be able to check that the two dimensions actually belong to the same cube. As above, we merely verify that they the related dimension is use in a cube, not necessarily the same cube. That would potentially cause false-positive results if a graph contained multiple cubes. But since we typically only validate cubes individually, we should be covered

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a reasonable restriction. But I have an additional sh:property here:

 [
      sh:path
          (
              meta:relatesTo
              [ sh:inversePath sh:path ] 
           ) ;
      sh:class cube:MeasureDimension ;
      sh:message "value of meta:relatesTo must point to measure dimension " ;
   ]

The dimension which is related to should be a cube:MeasureDimension. I can't see a reasonable case, where that would not be the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


Improve validation of `meta:dimensionRelation`:

1. Check that upper/lower bound has at most one `dcterms:type`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kronmar I'm a little creative here since this was not really discussed. WDYT?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like reasonable checks

@tpluscode
Copy link
Contributor Author

I have not other ideas what can be validated about upper/lower limit. Ideas welcome @kronmar @Rdataflow

Copy link

@Kronmar-Bafu Kronmar-Bafu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small addition for :DimensionRelation altough open for discussion if such restriction would be overkill.

Rest looks good to me!


Improve validation of `meta:dimensionRelation`:

1. Check that upper/lower bound has at most one `dcterms:type`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like reasonable checks

validation/standalone-constraint-constraint.ttl Outdated Show resolved Hide resolved
Comment on lines 194 to 204
[
sh:path
(
meta:relatesTo
[ sh:inversePath sh:path ]
[ sh:inversePath sh:property ]
) ;
sh:minCount 1 ;
sh:class cube:Constraint ;
sh:message "value of meta:relatesTo must be a cube dimension" ;
] ;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a reasonable restriction. But I have an additional sh:property here:

 [
      sh:path
          (
              meta:relatesTo
              [ sh:inversePath sh:path ] 
           ) ;
      sh:class cube:MeasureDimension ;
      sh:message "value of meta:relatesTo must point to measure dimension " ;
   ]

The dimension which is related to should be a cube:MeasureDimension. I can't see a reasonable case, where that would not be the case.

@ktk
Copy link
Member

ktk commented Sep 30, 2024

@Kronmar-Bafu could you check again, I would like to see this merged.

@tpluscode tpluscode merged commit 2a93676 into main Sep 30, 2024
25 checks passed
@tpluscode tpluscode deleted the error-margin-validation branch September 30, 2024 10:17
@Kronmar-Bafu
Copy link

@ktk Sorry, was on vacation. Looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants