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

Fixing several small documentation bugs #385

Merged
merged 10 commits into from
Jul 4, 2024

Conversation

ielis
Copy link
Contributor

@ielis ielis commented Apr 5, 2024

@julesjacobsen
The PR fixes several small inaccuracies in the documentation, mostly deviations from the protobuf schema.

Fixes #316, #339, #344, #350, #351

Copy link
Collaborator

@pnrobinson pnrobinson left a comment

Choose a reason for hiding this comment

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

Looks fine but there is not reason for the change in interpretation/Miller syndrome

@ielis
Copy link
Contributor Author

ielis commented Jul 4, 2024

Hi @pnrobinson
I will fix this soon, but there is one more thing I'd like to check. Should we italicize gene symbols? Currently, the documentation does not italicize human gene symbols.

@pnrobinson
Copy link
Collaborator

gene symbols should alsways be italicized!

@ielis
Copy link
Contributor Author

ielis commented Jul 4, 2024

@pnrobinson I went through the docs to italicize the gene symbols (where appropriate, not in a publication title, or in a hyperlink target) and while doing that I also fixed a few typos.

Please review, thank you!

@ielis ielis requested a review from pnrobinson July 4, 2024 12:05
@@ -150,7 +150,7 @@ with the hypothetical gene YFG42.
id: "OMIM:263750"
label: "Miller syndrome"
genomicInterpretations:
- interpretationStatus: "CONTRIBUTORY"
- interpretationStatus: "CANDIDATE"
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case the interpretationStatus should be CAUSAL, this was a published 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.

Well, it may have been published, but is that relevant in the context of this example and, specifically, in the context of the previous paragraph?

Research consortia may exchange information about candidate genes in which an undisclosed
variant was found that was assessed to be possibly related to a disease or phenotype but
for which insufficient evidence is available to be certain. The intention is often to find
other researchers with similar cases in order to subsequently share detailed information in
a collaborative project.

Based on the table below, shouldn't the status be set to CANDIDATE, if "the research consortium found a variant with insufficient evidence"?
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it does not fit well with the description, but it si not wrong, I will merge

Copy link
Collaborator

@pnrobinson pnrobinson 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 change requested then we can merge!

@pnrobinson pnrobinson merged commit d69fc9e into phenopackets:master Jul 4, 2024
6 of 7 checks passed
@ielis ielis deleted the fix-doc-bugs branch July 4, 2024 18:33
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.

link to protobuf 3 incorrect / not available
2 participants