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

Add min/max value constraints to score and confidence slots. #373

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

gouttegd
Copy link
Contributor

Resolves #372

  • docs/ have been added/updated if necessary (not applicable; the doc was already ahead of the model, this PR is about updating the model to align it with the doc)
  • make test has been run locally
  • tests have been added/updated (not applicable)
  • CHANGELOG.md has been updated.

The description of the confidence, semantic_similarity_score, and similarity_score slots says that those slots expect a value between 0 and 1, but the model does not enforce that. LinkML provides a way to formalise this kind of value constraints (which could then be automatically enforced by a runtime that is aware of those constraints), so we do exactly that here: we add minimum_value and maximum_value constraints to the aforementioned slots.

The description of the `confidence`, `semantic_similarity_score`, and
`similarity_score` slots says that those slots expect a value between
0 and 1, but the model does not enforce that. LinkML provides a way to
formalise this kind of value constraints (which could then be
automatically enforced by a runtime that is aware of those constraints),
so we do exactly that here: we add `minimum_value` and `maximum_value`
constraints to the aforementioned slots.
@gouttegd gouttegd self-assigned this Jul 18, 2024
@gouttegd gouttegd added the enhancement New feature or request label Jul 18, 2024
@gouttegd gouttegd requested a review from matentzn July 18, 2024 13:38
Copy link
Collaborator

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

We probably need a better SOP for testing these kinds of PR. I had been working on a pipeline, but the truth is, what would actually need to happen is:

  1. Build LinkML model (this ensures that the model is syntactically well-defined)
  2. Build version of sssom py with that locally cached linkml model
  3. Run sssom-py test suites

Else I dont know if a change like this will break existing tests. Did you test these changes somehow?

@gouttegd
Copy link
Contributor Author

  1. Build LinkML model (this ensures that the model is syntactically well-defined)

I believe the existing test suite here (the one that is run by make test) takes care of validating the model at least from a syntactical standpoint. First, because what would even be the point of the test suite if it did not even do that, and second, because when preparing this PR I initially and erroneously wrote maximal_value instead of maximum_value, and the test suite (which I ran locally before even committing any changes) correctly rebuked me for that.

  1. Build version of sssom py with that locally cached linkml model
  2. Run sssom-py test suites

Why would we want sssom to fail because of problems that could exist in sssom-py?

If SSSOM-Py breaks when the model enforces a constraint that has always been a part of the spec but was not previously enforced, then it's a problem with SSSOM-Py, not with the spec.

@matentzn
Copy link
Collaborator

I believe the existing test suite here (the one that is run by make test) takes care of validating the model at least from a syntactical standpoint.

You are right, forgot about that; it already does what I thought it did not.

@matentzn matentzn requested a review from hrshdhgd July 19, 2024 05:14
@gouttegd gouttegd merged commit 73bffc3 into master Jul 19, 2024
3 checks passed
@gouttegd gouttegd deleted the add-min-max-constraints branch July 19, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Range constraints for Double-typed slots not specified/enforced
3 participants