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

Feature 1419 competency question coverage report #1420

Merged
merged 45 commits into from
Aug 1, 2023

Conversation

areleu
Copy link
Contributor

@areleu areleu commented Nov 24, 2022

Summary of the discussion

This is the branch I will work on for a coverage report. It builds on #1319

Type of change (CHANGELOG.md)

Added

  • Added a new class #

Updated

  • Updated a definition #

Removed

  • Removed a broken link #

Workflow checklist

Automation

Closes #1419

PR-Assignee

Reviewer

  • 🐙 Follow the Reviewer Guide
  • 🐙 Provided feedback and show sufficient appreciation for the work done

@github-actions github-actions bot added the oeo-physical changes the oeo-physical module label Nov 24, 2022
@areleu areleu added this to the oeo-release-1.14.0 milestone Nov 24, 2022
@areleu areleu self-assigned this Nov 24, 2022
@areleu
Copy link
Contributor Author

areleu commented Nov 25, 2022

I am still to implement the badge. There is a very important caveat here. The tests can't run with the owl file because for some reason the translation is not correctly pasing some labels. I have to look at it from a linux computer because my work pc is windows and I have to do a lot of things from imagination.

Related issue: #1424

@areleu
Copy link
Contributor Author

areleu commented Nov 28, 2022

Im going to be working on implementing the badge in a personal fork because I have to play around with the secrets and such. When I have it figure out I will come to you and tell you exactly the steps to be followed

@areleu
Copy link
Contributor Author

areleu commented Nov 28, 2022

Take a look at the README of this fork branch, that is how the badge would look like:

https://github.com/areleu/ontology/tree/feature-1419-competency-question-coverage-report

To implement it here I would need to support from someone with owner rights to set up the secrets correctly etc.

@areleu
Copy link
Contributor Author

areleu commented Nov 28, 2022

Now the tests fail if the competency questions are not fulfiled. Its missing a way of ruling out CQs. But In general I would encourage against that.

Something that I like from this implementation is that you get feedback in the CI itself on which tests failed.

@l-emele l-emele self-requested a review June 15, 2023 12:45
@l-emele
Copy link
Contributor

l-emele commented Jun 15, 2023

Looking at this PR, where did we decide to rename all the files? This PR goes far beyond what was discussed in the issue.
EDIT: I always understood the question numbers like persistent IDs.

@areleu
Copy link
Contributor Author

areleu commented Jun 15, 2023

Looking at this PR, where did we decide to rename all the files? This PR goes far beyond what was discussed in the issue.

It was part of the proposed structure:

|---> completeness
    |---> shared
        |---> competency_question_with_explicit_wording.omn
          ...
    |---> model
     ...
    |---> social
     ...
    |---> physical
     ...
|---> soundness
 ...

But I can easily revert back to the old one and it should still work, what I would definitively do if we dont revert is make sure that the numbering is consistent with the old numbering, for traceability.

@areleu
Copy link
Contributor Author

areleu commented Jun 15, 2023

Looking at this PR, where did we decide to rename all the files? This PR goes far beyond what was discussed in the issue. EDIT: I always understood the question numbers like persistent IDs.

I made sure the numbers now are consistent with the old ones. Also restored deleted questions, even the duplicates.

@l-emele
Copy link
Contributor

l-emele commented Jun 16, 2023

How does run_questions.sh work after the PR? This script still uses the old file names.

This PR changes too much things at once, this makes it very hard to review.


# This competency question gets deprecated with this pull request: https://github.com/OpenEnergyPlatform/ontology/pull/1409
# This means it should be false
Copy link
Contributor

Choose a reason for hiding this comment

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

To me, the answer this competency question is neither false not right, but the question is undecidable, because one needs additional information about the biofuel to decide this question. Same with Q29.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I will handle this differently, I will add a folder where all the deprecated questions are to me moved, without changing their conditions at all. I will also add the option to run the deprecated questions by adding the "--deprecated" flag to the pytest call.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is a good solution.

@areleu
Copy link
Contributor Author

areleu commented Jun 19, 2023

How does run_questions.sh work after the PR? This script still uses the old file names.

This PR changes too much things at once, this makes it very hard to review.

This file can be deleted, test_competency_questions.py replaces it

@l-emele
Copy link
Contributor

l-emele commented Jun 19, 2023

This file can be deleted, test_competency_questions.py replaces it

Are any of the changes of this PR documented? I do not see any entries in CHANGELOG.md. In the past we documented there also changes to the competency questions and scripts, e.g.

- IAO extraction file iao-extracted.owl and extraction scripts extract-iao-module.sh, iao-w-hierarchy.txt (#1555)

- biofuel; competency questions Q1 and Q2 (#1409)

@chrwm
Copy link
Member

chrwm commented Jul 27, 2023

How is the status and this PR and should it be considered for the release 1.16.0 next Tuesday? @areleu

@areleu
Copy link
Contributor Author

areleu commented Jul 31, 2023

How is the status and this PR and should it be considered for the release 1.16.0 next Tuesday? @areleu

I can rebase it and it will be ready

@areleu areleu merged commit 79d373c into dev Aug 1, 2023
@areleu areleu deleted the feature-1419-competency-question-coverage-report branch August 1, 2023 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
oeo-physical changes the oeo-physical module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There is no competency question coverage report
4 participants