Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

refactor!: clean up app #474

Merged
merged 120 commits into from
Aug 7, 2023
Merged

refactor!: clean up app #474

merged 120 commits into from
Aug 7, 2023

Conversation

korikuzma
Copy link
Member

@korikuzma korikuzma commented Jul 27, 2023 •
edited
Loading

Close #162, #332, #429, #119, #428, #189, #309, #414, #475, #427

@jsstevenson I'm realllly sorry for making this large of a PR. I'm going to open now. There's some places (validators/translators) where I want to use shared methods for DRY principle. However, I'm going to open up now since it's so large and look at doing this while you review.

Notes:

  • Mainly focused on cleanup related to to_vrs and normalize endpoints. Did not really look at gnomad_vcf_to_protein or copy_number_variation modules
  • Remove to canonical variation (no longer support)
  • Combined tests for tokenizers/classifiers/validators/translators into one module
  • Removed amino_acids.csv (accidentally left in)
  • Names changes
    • Coding DNA → cDNA
    • Polypeptide truncation → Protein Stop Gain
    • Silent Mutation → Reference Agree
    • Uncertain/Range → Ambiguous
    • HGVSDupDelModeEnum → HGVSDupDelModeOption
  • Validators no longer do any kind of translations to VRS representations. Translators will do this work
  • Classifier only returns exact matches and only returns a single classification rather than a list
  • Use regex patterns (in variation/regex.py) rather than multiple if/else conditions
  • Remove unused code
  • Create variation schemas for supported variation types. Uses consistent field naming
  • Cleaning up instance variables in classes
  • Only run fully justified allele normalization on VRS Alleles. Do not run on VRS Copy Number
  • Pulled tokenize, classify, validate, translate outside of subdirectories (variation/tokenizers, variation/classifiers, variation/validators, variation/translators) and moved to app root
  • baseline_copies is required in /hgvs_to_copy_number_count
  • cool-seq-tool update
    • Removes file path params from QueryHandler, can set these via environment variables
    • QueryHandler accepts only uta_db_url as param and removes uta_db_pwd
  • new dependencies for linting
    • ruff (replaced flake8)
    • black

korikuzma added 30 commits May 10, 2023 10:28
@korikuzma
refactor: update TokenType + remove protein termination classifier
b17d52c
@korikuzma
refactor: Remove TokenMatchType (not necessary)
008b632
@korikuzma
forgot to remove additional match_type
c2a47bb
@korikuzma
refactor: clean up handling unknown tokens
111d104
@korikuzma
refactor: Rename GeneMatchToken to GeneToken
cc79fd1
@korikuzma
refactor: create AltType str enum
5cd918d
@korikuzma
refactor: update TokenType type
095c9e9
@korikuzma
refactor: remove LookupType enum (not used)
b79410c
@korikuzma
wip: add work for tokenizers
67ab56a
@korikuzma
wip: storing progress
944d6db
@korikuzma
wip: store progress for delins
a4c2bf7
@korikuzma
wip: store progress for cdna insertion
8b76419
@korikuzma
wip: store progress for ref agree
7857767
@korikuzma
wip: minor cleanup of validators
6778521
@korikuzma
wip: store progress for protein stop gain
d567c5a
@korikuzma
wip: store initial work for genomic dup
59e7942
@korikuzma
wip: store progress for genomic ambiguous dups
4dd8043
@korikuzma
wip: progress for genomic del
b652db8
@korikuzma
wip: remove canonical variation work
d2cf750
@korikuzma
wip: fix classifiers
d909427
@korikuzma
wip: handle if mane none in translators
80dd57c
@korikuzma
wip: tmp stop running gh actions
1b39114
@korikuzma
wip: more progress for normalize
8dd9d92
@korikuzma
wip: store more progress
df8e845
@korikuzma
wip: add genomic del ambiguous
450981e
@korikuzma
iMerge branch 'main' into issue-332-kori-merge-main
8310cdf
@korikuzma
wip: fix tokenizers
8512eea
@korikuzma
wip: clean up classifier tests
6ee5110
@korikuzma
wip: rename coding dna --> cdna
666ab23
@korikuzma
wip: storing progress for dup1
f194615
@korikuzma
Copy link
Member Author

@jsstevenson Thanks for all your feedback so far! So happy to get another set of eyes to find things I've missed

@korikuzma
pr review changes
1bb95ae 
- Remove unused classification_type method
- Update type hints / return types
- Remove del_or_dup and use AltType
@korikuzma
Copy link
Member Author

korikuzma commented Aug 1, 2023 •
edited
Loading

Might look to see about removing AltType and seeing if we can use an existing field in the models. Also think I may remove CoordinateType and just use MoleculeContext. Although I think molecule context is being removed once we move to 2.0-alpha (at least I don't see it in the schema yet).

This was referenced Aug 1, 2023
Closed
Open
korikuzma
korikuzma commented Aug 2, 2023
View reviewed changes
README.md Show resolved Hide resolved
@korikuzma korikuzma mentioned this pull request Aug 2, 2023
Closed
@jsstevenson jsstevenson self-requested a review August 2, 2023 13:30
jsstevenson
jsstevenson approved these changes Aug 2, 2023
View reviewed changes
Copy link
Member

@jsstevenson jsstevenson left a comment

Choose a reason for hiding this comment

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

@korikuzma astounding work! I think I'm caught up through f422b4d, so I'll just do an approve now in the interest of keeping this moving

@korikuzma korikuzma changed the base branch from main to refactor August 2, 2023 13:47
jsstevenson
jsstevenson reviewed Aug 2, 2023
View reviewed changes
Pipfile Show resolved Hide resolved
korikuzma
korikuzma commented Aug 6, 2023
View reviewed changes
Copy link
Member Author

@korikuzma korikuzma left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Need to make this change, otherwise RSE default won't work as expected. Should add a test for dup > 100 bps
Screenshot 2023-08-06 at 5 51 40 PM

@korikuzma korikuzma mentioned this pull request Aug 7, 2023
Open
@korikuzma
Copy link
Member Author

Adding todo's in #163 . Going to merge this into refactor

@korikuzma korikuzma merged commit 77d3ef3 into refactor Aug 7, 2023
@korikuzma korikuzma deleted the issue-332-kori branch August 7, 2023 20:30
@korikuzma korikuzma mentioned this pull request Aug 21, 2023
Merged
korikuzma added a commit that referenced this pull request Aug 25, 2023
@korikuzma
refactor!: clean up app (metaschema) (#494)
043285a 
- Refactor app (#474)
  - Mainly focused on cleanup related to to_vrs and normalize endpoints. Did not really look at gnomad_vcf_to_protein or copy_number_variation modules
  - Remove to canonical variation (no longer support)
  - Combined tests for tokenizers/classifiers/validators/translators into one module
  - Removed amino_acids.csv (accidentally left in)
  - Names changes
    - Coding DNA → cDNA
    - Polypeptide truncation → Protein Stop Gain
    - Silent Mutation → Reference Agree
    - Uncertain/Range → Ambiguous
    - HGVSDupDelModeEnum → HGVSDupDelModeOption
  - Validators no longer do any kind of translations to VRS representations. Translators will do this work
  - Classifier only returns exact matches and only returns a single classification rather than a list
  - Use regex patterns (in variation/regex.py) rather than multiple if/else conditions
  - Remove unused code
  - Create variation schemas for supported variation types. Uses consistent field naming
  - Cleaning up instance variables in classes
  - Only run fully justified allele normalization on VRS Alleles. Do not run on VRS Copy Number
  - Pulled tokenize, classify, validate, translate outside of subdirectories (variation/tokenizers, variation/classifiers, variation/validators, variation/translators) and moved to app root 
  - baseline_copies is required in /hgvs_to_copy_number_count
  - cool-seq-tool update
      - Removes file path params from QueryHandler, can set these via environment variables
      - QueryHandler accepts only uta_db_url as param and removes uta_db_pwd
   - new dependencies for linting
    - ruff (replaced flake8)
    - black 
- Add more support for gnomad vcf expressions in normalize (#479, #489)
- Remove pyliftover from deps (covered by cool-seq-tool) (#480) 
- Fix default mode for hgvs dup del mode wrt rse (#482)
- Fix default HGVS dup del mode - dels should be allele w lse (#484)
- Use cool-seq-tool AnnotationLayer and rm CoordinateType (#485)
- Remove structural type from varaition descriptor (#487)
@korikuzma korikuzma mentioned this pull request Aug 25, 2023
Closed
korikuzma added a commit that referenced this pull request Sep 22, 2023
@korikuzma
refactor!: clean up app (#474)
6156419 
- Mainly focused on cleanup related to to_vrs and normalize endpoints. Did not really look at gnomad_vcf_to_protein or copy_number_variation modules
- Remove to canonical variation (no longer support)
- Combined tests for tokenizers/classifiers/validators/translators into one module
- Removed amino_acids.csv (accidentally left in)
- Names changes
  - Coding DNA → cDNA
  - Polypeptide truncation → Protein Stop Gain
  - Silent Mutation → Reference Agree
  - Uncertain/Range → Ambiguous
  - HGVSDupDelModeEnum → HGVSDupDelModeOption
- Validators no longer do any kind of translations to VRS representations. Translators will do this work
- Classifier only returns exact matches and only returns a single classification rather than a list
- Use regex patterns (in variation/regex.py) rather than multiple if/else conditions
- Remove unused code
- Create variation schemas for supported variation types. Uses consistent field naming
- Cleaning up instance variables in classes
- Only run fully justified allele normalization on VRS Alleles. Do not run on VRS Copy Number
- Pulled tokenize, classify, validate, translate outside of subdirectories (variation/tokenizers, variation/classifiers, variation/validators, variation/translators) and moved to app root 
- baseline_copies is required in /hgvs_to_copy_number_count
- cool-seq-tool update
    - Removes file path params from QueryHandler, can set these via environment variables
    - QueryHandler accepts only uta_db_url as param and removes uta_db_pwd
- new dependencies for linting
  - ruff (replaced flake8)
  - black
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jsstevenson jsstevenson jsstevenson approved these changes

Assignees

@korikuzma korikuzma

Labels
priority:high High priority technical debt A feature/requirement implemented in a sub-optimal way & must be re-written. Contrast to "cleanup"
Projects
None yet
Milestone
No milestone
2 participants
@korikuzma @jsstevenson