Conversation
removed test file
There was a problem hiding this comment.
For this PR, let's revert to the original version
There was a problem hiding this comment.
@lupoglaz Is this a fix for the "master" branch or something you needed for the "dev branch? In other words, is it going to break the "master" code if we don't use your local version of CUSP?
There was a problem hiding this comment.
I believe we just need a version of CUSP in the TPL directory when it's compiled locally.
There was a problem hiding this comment.
In function "Angles2Coords_forward", you have two "if" blocks (polymer_type 0 and polymer_type 1 or 2) but it seems the code is identical except for "<8" for proteins and "<10" for nucleic acids. So you can use the "if" clause for that little piece of code only:
if(polymer_type == 0){
if( length<seq.length() || single_angles.sizes()[0]<8 ){
ERROR("incorrect angles tensor length");
}
} else if(polymer_type == 1 or polymer_type == 2) {
if( length<seq.length() || single_angles.sizes()[0]<10 ){
ERROR("incorrect angles tensor length");
}
}
That way, no need to duplicate the code.
As far as I can see, the call to function "ProtUtil::getNumAtoms(seq, add_terminal)" for proteins can be replaced by the version you have for nucleic acids: "ProtUtil::getNumAtoms(seq, add_terminal, polymer_type, chain_names)".
There was a problem hiding this comment.
Lots of duplicated code between the "protein" and the "nucleic acid" parts... If anything can be done to reduce the redundancy (without compromising the readability), that would be an improvement
There was a problem hiding this comment.
Lots of C++ indentation issues...
Also, whenever you have a series of mutually exclusive "if" clauses, you're better with either the "switch-case" construct "if else if else if" construct. Faster and cleaner...
There was a problem hiding this comment.
It's good to keep the old values in the code -- just commented out.
Provide the DOIs of the articles in the comments, though:
PeptideBuilder: DOI:10.7717/peerj.80
Engh and Huber: DOI:10.1107/S0108767391001071
By the way, some of the values are quite different between the two. Were some of these bugs? Make sure you are not introducing a bug yourself, @fmatreale
As always, fix the indentation...
There was a problem hiding this comment.
Fix the indentation...
I don't understand what the "model_count" variable is doing. Why do you read the data only if "model_count" is 1.
As usual, try to remove the duplicate code between the protein case, the RNA case, and the DNA case.
There was a problem hiding this comment.
This looks like a benchmark for code that's already in TPL, so it can be merged.
There was a problem hiding this comment.
@fmatreale Not a change we need for nucleic acids. Revert to the original master branch
There was a problem hiding this comment.
@fmatreale Not a change we need for nucleic acids. Revert to the original master branch
There was a problem hiding this comment.
@fmatreale Not a change we need for nucleic acids. Revert to the original master branch
There was a problem hiding this comment.
@fmatreale Not a change we need for nucleic acids. Revert to the original master branch
There was a problem hiding this comment.
@fmatreale Not a change we need for nucleic acids. Revert to the original master branch
There was a problem hiding this comment.
@fmatreale Not a change we need for nucleic acids. Revert to the original master branch
There was a problem hiding this comment.
@fmatreale Not a change we need for nucleic acids. Revert to the original master branch
There was a problem hiding this comment.
@fmatreale Not a change we need for nucleic acids. Revert to the original master branch
There was a problem hiding this comment.
@fmatreale Not a change we need for nucleic acids. Revert to the original master branch
There was a problem hiding this comment.
@fmatreale Not a change we need for nucleic acids. Revert to the original master branch
There was a problem hiding this comment.
@lupoglaz, what should we keep from these changes if we don't want the "Physics" code yet? Is "Volume/VolumeMultiplication" ready for release? What about "Utils/Protein" and "Utils/Volume"? If you could make a version of that file that we can use for release, that would be appreciated :-)
There was a problem hiding this comment.
@lupoglaz It doesn't look like it's ready for release. Can you confirm that it's OK to not include that whole file for the time being? (along with the ".h" file, I guess)
There was a problem hiding this comment.
@lupoglaz For that whole Kernel/HashKernel issue, can you modify the code in the "na" branch so that it can be merged to the "master" branch? It seems this code still belongs just to "dev"
There was a problem hiding this comment.
@lupoglaz For that whole Kernel/HashKernel issue, can you modify the code in the "na" branch so that it can be merged to the "master" branch? It seems this code still belongs just to "dev"
There was a problem hiding this comment.
@lupoglaz Georgy, can you confirm that the "Coords2Elect" code can be removed from the "na" branch? @fmatreale can take care of that
There was a problem hiding this comment.
@lupoglaz Georgy, can you confirm that the "Coords2Elect" code can be removed from the "na" branch? @fmatreale can take care of that
There was a problem hiding this comment.
@lupoglaz Georgy, can you confirm that the "Coords2Stress" code can be removed from the "na" branch? @fmatreale can take care of that
There was a problem hiding this comment.
@lupoglaz Georgy, can you confirm that the "Coords2Stress" code can be removed from the "na" branch? @fmatreale can take care of that
There was a problem hiding this comment.
@fmatreale Remove from the "na" branch. It's definitely not something we need to merge to "master".
glamoure
left a comment
glamoure
left a comment
There was a problem hiding this comment.
@fmatreale, unless Georgy confirms that the new "HashKernel" code is working, would you mind reverting to the original?
There was a problem hiding this comment.
@lupoglaz Is that code ready for release? In other words, is the "HashKernel" code ready?
There was a problem hiding this comment.
Please, check the latest commit to the dev branch. It compiles and to test HashKernel you can:
cd UnitTests/Volume/TypedCoords2Volume
python test_forward.py
afterwards, you'll have xplor in UnitTests/Volume/TypedCoords2Volume/TestFig. However I can't guarantee correctness of backward pass. You can try to test it yourself, but I recommend overwriting everything.
There was a problem hiding this comment.
@lupoglaz Is that code ready for release? In other words, is the "HashKernel" code ready?
There was a problem hiding this comment.
@fmatreale Can you clarify the comments? These are Python reimplementations of some C++ functions, right?
| } | ||
|
|
||
|
|
||
| // AS: Need transformation matrices (setDihedral,setDihedralDphi,setDihedralDpsi,setDihedralDr, setRx, setRy, setRz, setDRx) |
There was a problem hiding this comment.
Is this file even needed?
| if polymer_type == 1 or polymer_type == 2: | ||
| ctx.save_for_backward(input_angles_cpu, sequenceTensor, torch.tensor(polymer_type, dtype=torch.int32), chain_names) | ||
| input_angles_cpu = input_angles_cpu.contiguous() | ||
| max_num_atoms = num_atoms |
There was a problem hiding this comment.
Why not "torch.max(num_atoms)" like for polymer_type 0? This code assumes "num_atoms" is a single number, not an array.
| '''Converts a string to 0-terminated byte tensor''' | ||
| return torch.from_numpy(np.fromstring(string+'\0', dtype=np.uint8)) | ||
|
|
||
| def convert2str(tensor): |
There was a problem hiding this comment.
Is this function needed? I don't see it being called anywhere in that file. (Somewhere else, maybe?)
| current_residue.add(atom) | ||
|
|
||
| if polymer_type == 2: | ||
| if len(atom_name) == 0: |
There was a problem hiding this comment.
Why is this needed only for polymer_type 2?
|
|
||
| if polymer_type == 1 or polymer_type == 2: | ||
| # print(str(residue)[10]) | ||
| if str(residue)[10] == 'A' or str(residue)[9] == 'A': |
There was a problem hiding this comment.
Is this the best way to implement this? Why is it sometimes at position 9 and sometimes at position 10?
There was a problem hiding this comment.
Part of the "physics" addition. Do not include
There was a problem hiding this comment.
Part of the "physics" addition. Do not include
There was a problem hiding this comment.
Part of the "physics" addition. Do not include
There was a problem hiding this comment.
Why is this file in the repo?
There was a problem hiding this comment.
@fmatreale Not needed, since we don't merge that code in the "master" branch
There was a problem hiding this comment.
@fmatreale Not needed, since we don't merge that code in the "master" branch
There was a problem hiding this comment.
@fmatreale Not needed, since we don't merge that code in the "master" branch
There was a problem hiding this comment.
@fmatreale Not needed, since we don't merge that code in the "master" branch
There was a problem hiding this comment.
@fmatreale Not needed, since we don't merge that code in the "master" branch
There was a problem hiding this comment.
@fmatreale Not needed, since we don't merge that code in the "master" branch
There was a problem hiding this comment.
@fmatreale Not needed, since we don't merge that code in the "master" branch
There was a problem hiding this comment.
@fmatreale Not needed, since we don't merge that code in the "master" branch
There was a problem hiding this comment.
@fmatreale Not needed, since we don't merge that code in the "master" branch
There was a problem hiding this comment.
@fmatreale Not needed, since we don't merge that code in the "master" branch
5 participants
TorchProteinLibrary 0.2 Nucleic Acid Extension