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.

Sign up for GitHub

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

Jump to bottom

MHCADDI model #74

Merged
merged 22 commits into from Feb 7, 2022
Merged

MHCADDI model #74

merged 22 commits into from Feb 7, 2022

Conversation

sebastiandro
Copy link
Collaborator

@sebastiandro sebastiandro commented Feb 3, 2022 •
edited by benedekrozemberczki

Closes #13

Summary

Please provide a high-level summary of the changes for the changes and notes for the reviewers

  • Unit tests provided for these changes
  • Documentation and docstrings added for these changes using the sphinx style

Changes

! This is a draft, the model still need a bit of refactoring, documentation etc

@benedekrozemberczki
Copy link
Contributor

@sebastiandro I will look into the segmentation indices.

@cthoyt cthoyt added the model label Feb 3, 2022
@cthoyt cthoyt mentioned this pull request Feb 3, 2022
Closed
@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2022 •
edited

Codecov Report

Merging #74 (54143d4) into main (7cb1533) will increase coverage by 0.63%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
+ Coverage   95.11%   95.75%   +0.63%     
==========================================
  Files          30       32       +2     
  Lines        1188     1366     +178     
==========================================
+ Hits         1130     1308     +178     
  Misses         58       58
Impacted Files Coverage Δ
chemicalx/models/mhcaddi.py 100.00% <100.00%> (ø)
chemicalx/utils.py 100.00% <100.00%> (ø)
tests/unit/test_models.py 100.00% <100.00%> (ø)
tests/unit/test_utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cb1533...54143d4. Read the comment docs.

cthoyt
cthoyt reviewed Feb 4, 2022
View reviewed changes
chemicalx/models/mhcaddi.py Outdated Show resolved Hide resolved
cthoyt
cthoyt reviewed Feb 4, 2022
View reviewed changes
chemicalx/models/mhcaddi.py Outdated Show resolved Hide resolved
cthoyt
cthoyt reviewed Feb 4, 2022
View reviewed changes
chemicalx/models/mhcaddi.py Outdated Show resolved Hide resolved
@benedekrozemberczki
Copy link
Contributor

Will not merge without a complete rewrite of the variable names and docstrings.

cthoyt
cthoyt reviewed Feb 4, 2022
View reviewed changes
chemicalx/models/mhcaddi.py Outdated Show resolved Hide resolved
@cthoyt
Copy link
Collaborator

cthoyt commented Feb 4, 2022

After that's all done I'll take a other pass at review

@benedekrozemberczki benedekrozemberczki marked this pull request as ready for review February 5, 2022 14:42
@benedekrozemberczki
Copy link
Contributor

@cthoyt Simplified it massively.

cthoyt
cthoyt reviewed Feb 5, 2022
View reviewed changes
chemicalx/models/mhcaddi.py Outdated Show resolved Hide resolved
@cthoyt
Various cleanup
68c2118 
- use new-style `super().__init__()`
- improve some (but not all) type annotations
- Add FIXME
@cthoyt
Update chemicalx/models/mhcaddi.py
c4d2057
@cthoyt
Update pipeline.py
1c2f4d4
@benedekrozemberczki
Copy link
Contributor

@cthoyt can I merge this?

cthoyt
cthoyt reviewed Feb 6, 2022
View reviewed changes
chemicalx/models/mhcaddi.py Outdated Show resolved Hide resolved
cthoyt
cthoyt reviewed Feb 6, 2022
View reviewed changes
chemicalx/models/mhcaddi.py Outdated Show resolved Hide resolved
cthoyt
cthoyt reviewed Feb 6, 2022
View reviewed changes
chemicalx/models/mhcaddi.py Outdated Show resolved Hide resolved
cthoyt
cthoyt reviewed Feb 6, 2022
View reviewed changes
chemicalx/models/mhcaddi.py Outdated Show resolved Hide resolved
cthoyt
cthoyt reviewed Feb 6, 2022
View reviewed changes
chemicalx/models/mhcaddi.py Outdated Show resolved Hide resolved
cthoyt
cthoyt reviewed Feb 6, 2022
View reviewed changes
chemicalx/models/mhcaddi.py Outdated Show resolved Hide resolved
cthoyt
cthoyt reviewed Feb 6, 2022
View reviewed changes
examples/mhcaddi_examples.py Outdated Show resolved Hide resolved
cthoyt
cthoyt requested changes Feb 6, 2022
View reviewed changes
Copy link
Collaborator

@cthoyt cthoyt left a comment

Choose a reason for hiding this comment

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

Please address the TODOs in the code highlighted in the PR

@benedekrozemberczki benedekrozemberczki merged commit 9691445 into AstraZeneca:main Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benedekrozemberczki benedekrozemberczki benedekrozemberczki approved these changes

@cthoyt cthoyt cthoyt requested changes

Assignees
No one assigned
Labels
model
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add the MHCADDI model
4 participants
@sebastiandro @benedekrozemberczki @codecov-commenter @cthoyt