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

Create ratio_location.yaml #273

Merged
merged 29 commits into from
Jun 5, 2024
Merged

Create ratio_location.yaml #273

merged 29 commits into from
Jun 5, 2024

Conversation

ar-ibrahim
Copy link
Collaborator

@ar-ibrahim ar-ibrahim commented Jul 20, 2023

@ar-ibrahim ar-ibrahim requested a review from rays22 July 20, 2023 14:40
@ar-ibrahim ar-ibrahim self-assigned this Jul 20, 2023
rays22
rays22 previously requested changes Jul 21, 2023
Copy link
Contributor

@rays22 rays22 left a comment

Choose a reason for hiding this comment

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

We might want to change the name of the pattern, because it does not specify the location explicitly. The components attribute1 and attribute2 should specify and imply the location of the ratio-attribute defined here.

src/patterns/dosdp-patterns/ratio_location.yaml Outdated Show resolved Hide resolved
src/patterns/dosdp-patterns/ratio_location.yaml Outdated Show resolved Hide resolved
src/patterns/dosdp-patterns/ratio_location.yaml Outdated Show resolved Hide resolved
src/patterns/dosdp-patterns/ratio_location.yaml Outdated Show resolved Hide resolved
src/patterns/dosdp-patterns/ratio_location.yaml Outdated Show resolved Hide resolved
src/patterns/dosdp-patterns/ratio_location.yaml Outdated Show resolved Hide resolved
src/patterns/dosdp-patterns/ratio_location.yaml Outdated Show resolved Hide resolved
src/patterns/dosdp-patterns/ratio_location.yaml Outdated Show resolved Hide resolved
src/patterns/dosdp-patterns/ratio_location.yaml Outdated Show resolved Hide resolved
src/patterns/dosdp-patterns/ratio_location.yaml Outdated Show resolved Hide resolved
rays22 and others added 25 commits August 4, 2023 10:27
We can further improve the text later.
@ar-ibrahim
Copy link
Collaborator Author

ar-ibrahim commented Sep 18, 2023

@rays22 This is the suggested change to the name of the pattern, 'proportion_of_attributes', based on the PATO class 'proportionality_to'. I also created a tsv file for the pattern and generated a term as an example. I still think we may need to create an additional pattern where the variables are 'attribute1, attribute2, location'. This is to avoid creating a large number of new terms. For example if we want to create the ratio 'sputum sodium to sputum potassium ratio' we would need to create those two terms first. That would not be necessary if the variables include the location.

@ar-ibrahim ar-ibrahim dismissed rays22’s stale review October 3, 2023 10:45

Changed name of pattern, as suggested.

@ar-ibrahim ar-ibrahim marked this pull request as ready for review October 3, 2023 10:46
@ar-ibrahim
Copy link
Collaborator Author

I have consulted @rays22 about the recurring QC warnings and as per my understanding this has been an outstanding issue that does not prevent merging the PR should the suggested changes be approved.

@rays22 rays22 merged commit 04cac1e into master Jun 5, 2024
0 of 2 checks passed
@rays22 rays22 deleted the issue_272 branch June 28, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants