Conversation
3303023 to
59b7c48
Compare
5e87d6a to
96ed3d8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1896 +/- ##
==========================================
- Coverage 94.28% 89.96% -4.33%
==========================================
Files 214 215 +1
Lines 19000 19327 +327
==========================================
- Hits 17915 17388 -527
- Misses 1085 1939 +854
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a725cce to
fc98c2b
Compare
e92d58b to
339ef68
Compare
There was a problem hiding this comment.
Thanks @atravitz ! I added some comments regarding validation and error handling.
One thing that's currently missing is that the network planner always adds a SolventComponent to the ChemicalSystem, however the membrane case does not support a SolventComponent in the complex.
Once you add the protocol.validate to the network planner, this would also raise an Error.
42a42bb to
038177b
Compare
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @atravitz , this looks great! Left some small comments, the only important one is the catching of the actual error which would be very helpful to users I think.
| with runner.isolated_filesystem(): | ||
| result = runner.invoke(plan_rbfe_network, args) | ||
|
|
||
| assert result.exit_code == 1 |
There was a problem hiding this comment.
Would it make sense to directly test for the ComponentValidationError here?
| return protein_class.from_pdb_file(input_file) | ||
| elif _contains_any_substring(input_file, _PDBX_EXT): | ||
| return protein_class.from_pdbx_file(input_file) | ||
| except ValueError: |
There was a problem hiding this comment.
I think here I would catch and print the actual error. E.g. in the protein membrane case where no box vector is provided, the user wouldn't know why it was unable to load the file. The error would have said:
Error: Could not determine box_vectors. Please provide them explicitly via the ``box_vectors`` argument or enable ``infer_box_vectors``
So maybe something like this?
except ValueError as e:
click.secho(
f"Unable to load a {protein_class.__name__} from {input_file}.\nError: {e}",
err=True
)
| ): # this silences some stderr spam | ||
| return NOT_PARSED | ||
| def _contains_any_substring(input: str, substrings: Iterable[str]) -> bool: | ||
| return any([substring in input for substring in substrings]) |
There was a problem hiding this comment.
Would it be safer to check for the file ending? Though of course it could also be gziped, so I'm not sure. I was just thinking in case .pdb could just be part of the file name, without it being a pdb file?
| _ = _get_protein_membrane(filename, None) | ||
|
|
||
| captured = capsys.readouterr() | ||
| assert "Unable to load a ProteinMembraneComponent" in captured.err |
There was a problem hiding this comment.
If you add the actual Error to this, I would here also check for the box vector thing in the error.
47a2a03 to
f3d155d
Compare
|
No API break detected ✅ |
Checklist
newsentry, or the changes are not user-facing.pre-commit.ci autofix.Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).
Developers certificate of origin