Merged
Conversation
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…into noncovalent-interactions
…into noncovalent-interactions
lwalew
requested changes
Aug 13, 2025
Collaborator
lwalew
left a comment
There was a problem hiding this comment.
Lots of small nitpicks here, feel free to resolve them yourself. Just a number of small issues including:
- Some missing docs for the classes.
- Potentially an issue with units. If this is the case, I think it might be worth adding a test case that would catch this (indeed when I fix the units the output looks more reasonable - see image below)
- Number of minor code improvements. I think on the whole, I would rather break up some of the post-processing in the ui and add it to the benchmark page and include what is necessary in the results.
docs/source/api_reference/small_molecules/noncovalent_interactions.rst
Outdated
Show resolved
Hide resolved
tests/noncovalent_interactions/test_noncovalent_interactions.py
Outdated
Show resolved
Hide resolved
chrbrunk
reviewed
Aug 13, 2025
Collaborator
chrbrunk
left a comment
There was a problem hiding this comment.
@leonwehrhan. Looks really nice, I added my comments as well. Regarding the tests, please let me or @lwalew know if you want to quickly discuss in a call if details are unclear, I think all other things are really minor, but the unit tests we need to add a few I think.
src/mlipaudit/noncovalent_interactions/noncovalent_interactions.py
Outdated
Show resolved
Hide resolved
tests/noncovalent_interactions/test_noncovalent_interactions.py
Outdated
Show resolved
Hide resolved
lwalew
approved these changes
Sep 2, 2025
lwalew
pushed a commit
that referenced
this pull request
Sep 4, 2025
* feat: benchmark class * feat: unit test run model * feat: unit test for analyze * docs: add noncovalent interactions * fix: add license to conftest * feat: update main * feat: number of skipped structures as attribute * feat: refactor nci output * feat: refactor benchmark output * feat: ui * feat: ui skipped structures * chore: smaller test data * chore: unit tests for unallowed elements * chore: update tests * chore: update benchmark class * chore: update docs * chore: update ui * chore: linters
lwalew
pushed a commit
that referenced
this pull request
Sep 4, 2025
* feat: benchmark class * feat: unit test run model * feat: unit test for analyze * docs: add noncovalent interactions * fix: add license to conftest * feat: update main * feat: number of skipped structures as attribute * feat: refactor nci output * feat: refactor benchmark output * feat: ui * feat: ui skipped structures * chore: smaller test data * chore: unit tests for unallowed elements * chore: update tests * chore: update benchmark class * chore: update docs * chore: update ui * chore: linters
lwalew
pushed a commit
that referenced
this pull request
Sep 17, 2025
* feat: benchmark class * feat: unit test run model * feat: unit test for analyze * docs: add noncovalent interactions * fix: add license to conftest * feat: update main * feat: number of skipped structures as attribute * feat: refactor nci output * feat: refactor benchmark output * feat: ui * feat: ui skipped structures * chore: smaller test data * chore: unit tests for unallowed elements * chore: update tests * chore: update benchmark class * chore: update docs * chore: update ui * chore: linters
lwalew
pushed a commit
that referenced
this pull request
Sep 17, 2025
* feat: benchmark class * feat: unit test run model * feat: unit test for analyze * docs: add noncovalent interactions * fix: add license to conftest * feat: update main * feat: number of skipped structures as attribute * feat: refactor nci output * feat: refactor benchmark output * feat: ui * feat: ui skipped structures * chore: smaller test data * chore: unit tests for unallowed elements * chore: update tests * chore: update benchmark class * chore: update docs * chore: update ui * chore: linters
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
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.
Benchmark addition
Description
NCI benchmark is added.
Checklist
all abstract method implementations.
testing pattern.