Clarify sex encoding in Pedigree/Sex docs and Sex enum#442
Open
amcim wants to merge 2 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clarifies how sex is encoded in Phenopackets to resolve recurring confusion between the Phenopackets Sex enum ordinals
(FEMALE=1, MALE=2)and the PLINK/PED convention (1=male, 2=female). This update is purely documentation and code comment clarification. See #438 for the rationale behind the text of the written commentsindividual.proto: Added a comment above theSexenum that explains the integers are internal, do not match PED/PLINK encodings, and that Phenopackets should always use the string values (MALE, FEMALE) rather than integers.docs/sex.rst: Added a prominent warning stating that integers must not be used to encode sex, and that string values are requireddocs/pedigree.rst: Added the same warning. Clarified at the example that the sex column uses theSexencoding rather than PLINK convention. Removed a now-redundant statement that duplicated this note under the AffectedStatus section.Fixes #438
Fixes #391
Fixes #321 (Note that this issue is stale and was looking at an old version of the docs. The example is present and now displays correctly)
Additionally, I followed the conclusion in #391 which states that the PED example given should use phenopackets encoding for sex, it just needs to be clearly specified in the docs.