Feature: adding helper functions from SLAC #19
Conversation
There was a problem hiding this comment.
Thank you for this PR! Great work!
High-level comments before I take another look:
- We absolutely need good docstrings: these are primordial for other users (including code reviewers) to understand what you are doing -- without spending 1 hour on it. If a user needs to spend more than 20 minutes to understand 10 lines of code, they will leave and go to another package.
- We also really need unit-tests. @jedyeo you said that you would put the tests once the wrapper is merged in, but we do not need the wrapper for the unit tests associated to this PR. Unit tests test one simple function at a time, they do not test the whole pipeline. We need test functions such as:
test_mrc2data, test_data_and_dic_2_hdf5, etc, i.e. we need one test function per function implemented. This is why they are called "unit". - It is not good practice to put many functions in a file called "helper.py", because "helper.py" does not say much about what is inside. some companies forbid the use of filenames such as
utils.pyorhelpers.pyjust because people put everything in them. Please divide the code intoio.py(for changing formats and write/load data) andmath.py(for operations on rotations) files at least.
Could you address these? Then I can have a look at the code itself. Thank you!
simSPI/crd.py
Outdated
|
|
||
|
|
||
| def rotation_matrix_to_euler_angles(R): | ||
| """Compute Euler angles given a rotation matrix. |
There was a problem hiding this comment.
do you know what representation is assumed? ZYZ, ZXZ, etc.
There was a problem hiding this comment.
Thanks for your answer. This should go in the docstring.
simSPI/fov.py
Outdated
| Relative path to file containing topological information of particle | ||
| """ | ||
| traj = md.load(filename) | ||
| atom_indices = traj.topology.select("name CA or name P") |
There was a problem hiding this comment.
can the user select all atoms, or are just the alpha carbons selected. Some tests that use the same atoms type (not realistic with real proteins) would be good to ensure what atoms are actually giving turned in to an image.
There was a problem hiding this comment.
sorry did not notice this comment last time @geoffwoollard . I should not have hard-coded that in my notebooks in the past - this was only meant to deal with ribosomes in a reasonable amount of time. Ideally, you'd like the selection to be defined by the user - and by default all atoms should be selected.
There was a problem hiding this comment.
You need to have tests for basically every function. That way the automatic tests tell you where exactly something is failing. And you can also tests multiple expected inputs (big, small, etc).
Have a look at the tests for some of the code base. Then get writing and his that 90% and get those green checks!
|
@fredericpoitevin i changed the convention to ZYZ! |
There was a problem hiding this comment.
let's push this PR through and see how things come together with the unit tests with the other PRs compSPI/ioSPI#24 #32
If there are anymore issues that came up with future TODOs, I suggest you make issues of them so the ideas don't get lost track of
|
@jedyeo I think you changed to "zyz" not "ZYZ", right? @geoffwoollard looks like tests are not passing... |
|
@fredericpoitevin to address the failed tests, would it be better to revert the PR or to create a new PR addressing the tests? |
create new pr is best move imho, unless someone thinks otherwise |
|
@jedyeo and @arjunsingh3600 @geoffwoollard I'm not sure I agree. Would have been preferable to fix things in this PR prior merging. Now the problem will spread to other branches being created before it is solved. |
|
@fredericpoitevin I've created a revert PR request here. This should remove the code from the main branch while I push the test fixes. |
|
Thanks @arjunsingh3600 great initiative! |
@calhep @thisTyler @arjunsingh3600
This file contains some functions from the slac codebase that helps with the processing of particles. Tests won't cover this code yet but once we get the wrapper merged in (still cleaning that up) then we should be good.