-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix uncertain insertion #10
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10 +/- ##
==========================================
+ Coverage 89.03% 89.05% +0.01%
==========================================
Files 5 5
Lines 1824 1845 +21
Branches 127 129 +2
==========================================
+ Hits 1624 1643 +19
Misses 73 73
- Partials 127 129 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Yes. You can see it in the following archive site.
It got probably deprecated at some point. |
ba65e40
to
4307618
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for following the HGVS specification and preparing a repairer to migrate an old expression. 🙇
I have left a couple of comments, and also please bump the version up to 0.5.0-SNAPSHOT
.
src/clj_hgvs/repairer.cljc
Outdated
;; g.1134_1135ins(100) -> g.1134_1135insN[100] | ||
;; r.431_432ins(5) -> r.431_432insn[5] | ||
;; p.R78_G79ins23 -> p.R78_G79insX[23] | ||
(defn ^:no-doc replace-unknown-repeated-seq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description like insN[100]
does not mean repeated sequences. It expresses an uncertain inserted sequence. I feel like replace-uncertain-bases
is proper naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix in this commit: b551792
@@ -624,20 +624,28 @@ | |||
;;; e.g. g.6775delinsGA | |||
;;; g.6775delTinsGA | |||
;;; c.145_147delinsTGG | |||
;;; c.145_147delinsN[10] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though only a reference page of DNA deletion-insertion have an example of uncertain insertion, theoretically, RNA deletion-insertion and protein deletion-insertion can also have the uncertain insertion. Mutalyzer supports delinsn
, e.g. https://mutalyzer.nl/normalizer/NM_004006.2:r.2623_2803delinsn. Implementation of the uncertain insertion for only DNA deletion-insertion seems incomplete 🤔
4307618
to
36d828d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfection! Thank you so much 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review, @totakke! 🙏
I have no additional points to add. Thanks!
I fixed ins unknown repeat description.
I referred following format:
@totakke
g.1134_1135ins(100)
is older hgvs nomenclature?