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

Add support for theoretical and double antibodies in crossmatch api #1187

Merged

Conversation

kristinagalik
Copy link
Contributor

@kristinagalik kristinagalik commented Apr 11, 2023

Closes #1168

  • theoretical sa pridava aj ked neni double crossmatch, proste ako zvysne
  • double antibody sa pridava len ak ma double crossmatch

@kristinagalik kristinagalik marked this pull request as ready for review April 12, 2023 12:43
@kubantjan kubantjan requested review from jakubmonhart and abragtim and removed request for kubantjan April 12, 2023 15:28
@kubantjan
Copy link
Member

@kristinagalik davam se pryc, na prvni kolo prosim davat @abragtim a @jakubmonhart

@jakubmonhart jakubmonhart removed the request for review from abragtim April 13, 2023 11:17
Copy link
Collaborator

@jakubmonhart jakubmonhart left a comment

Choose a reason for hiding this comment

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

Podle mě to funguje tak jak má, takže ok. Mám jenom pár poznámek/dotazů:

  1. test_theoretical_and_double_antibodies_not_implemented bych přejmenoval, protože double už je naimplementovany a theoretical antibody tady v žádném match nefiguruje (a tedy se tady ani netestuje).
  2. Teď se tedy nikde netestuje jestli crossmatch api vrací správně theoretical antibody match že? (testuje se jenom že to správně dělá get_crossmatched_antibodies_per_group, tak je možná redundantní to testovat znovu jako výstup api když se theoretical match zpracovává stejně jako ostatní).
  3. Je potřeba u všech těch parsing issue specifikovat v assert tu value? (viz třeba ParsingIssueDetail.INSUFFICIENT_NUMBER_OF_ANTIBODIES_IN_HIGH_RES.value) Funguje to stejně když dáš jenom ParsingIssueDetail.INSUFFICIENT_NUMBER_OF_ANTIBODIES_IN_HIGH_RES a možná je to lehce lépe čitelné (to je asi fakt detail, ale když už jsem si toho všiml tak to radši píšu :) )
  4. viz comment u kódu

txmatching/web/api/crossmatch_api.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jakubmonhart jakubmonhart left a comment

Choose a reason for hiding this comment

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

Už jenom jedna poznámka v kódu, jinak ok.

tests/web/test_do_crossmatch_api.py Show resolved Hide resolved
@kubantjan kubantjan force-pushed the 1168_add_support_for_theoretical_and_double_antibodies_ branch from 46daae2 to 74adb2d Compare May 3, 2023 15:22
@kubantjan kubantjan merged commit 22613b4 into master May 3, 2023
@kubantjan kubantjan deleted the 1168_add_support_for_theoretical_and_double_antibodies_ branch May 3, 2023 15:23
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.

Add support for theoretical and double antibodies in crossmatch api
3 participants