Danpf/add resonance rcsb mmtf spec v1.1 #23
Conversation
Adds compatibility with mmtf v1.1 and adds to string methods for verbosity + debugging.
include/mmtf/binary_decoder.hpp
Outdated
| // | ||
| // Based on mmtf_c developed by Julien Ferte (http://www.julienferte.com/), | ||
| // Anthony Bradley, Thomas Holder with contributions from Yana Valasatava, | ||
| // Gazal Kalyan, Alexander Rose. | ||
| // Gazal Kalyan, Alexander Rose, and Daniel Farrell. |
There was a problem hiding this comment.
The idea with the "mmtf_c" part was to give credit to the people that worked on the rcsb/mmtf-c code. I would say that your name belongs to the "authors of this code" section further up... ;-)
And this may also be the case in many other files. Let me also use this to thank you very much for all your contributions!
There was a problem hiding this comment.
haha okay will do! You're welcome, it's a great project, I'm happy to contribute.
include/mmtf/structure_data.hpp
Outdated
| std::vector<int32_t> chainIndexList; | ||
| float matrix[16]; | ||
| std::vector<int32_t> chainIndexList; | ||
| std::vector<std::vector<float>> matrices; |
There was a problem hiding this comment.
I think this is a misunderstanding here. The matrix of the Transform object is a 4x4 array (i.e. 16 floats as it was before). See also here: https://github.com/rcsb/mmtf/blob/v1.0/spec.md/#bioassemblylist
The BioAssembly object contains a list of those Transform objects and this is already the case.
There was a problem hiding this comment.
You're totally right. I don't know what I was thinking!
include/mmtf/structure_data.hpp
Outdated
| groupName == c.groupName && | ||
| singleLetterCode == c.singleLetterCode && | ||
| chemCompType == c.chemCompType); | ||
| } | ||
|
|
||
| std::string | ||
| as_string() const { |
There was a problem hiding this comment.
what's the use-case of these to_string() methods? Debugging? How about implementing operator<< instead? The implementation could even go into a separate header (only declare a friend here) which is optional to include (e.g. <mmtf/debug.hpp>).
There was a problem hiding this comment.
Debugging, or general reporting.
I prefer to_string/as_string, that's really the only reason. We could move everything to a new file, but these functions don't really add much bloat. If you are passionate about the << operator or the extra file I can switch to that, i just think it is a messier implementation.
There was a problem hiding this comment.
Thanks @danpf. No really strong feelings here, but my general take:
- I think these methods should have documentation which states their purpose
- I think adding these methods should be a separate pull request
- I think
operator<<is cleaner
There was a problem hiding this comment.
ok I'll take out all the string methods before this gets merged!
There was a problem hiding this comment.
I agree with @speleo3. The operator<< is the standard C++ way for something like this and anything else would be confusing. Plus the implementation is almost identical since you are using streams anyways. If the string output looks decent, it can stay in the class as the standard string representation. Otherwise, I would also vote for having it in a separate header.
|
Good for review |
|
bump for this and other prs : ) |
|
@danpf my apologies for the delay. I have been extremely busy lately. I hope to get to this soon. Thanks for your patience. |
|
updated to be compatible with new master! |
There was a problem hiding this comment.
I am ok with all the changes and added some cleanup on top of it.
My only possible concern is whether any v1.0 compatible decoder can still read our files (see comment in #19). The only possible problem there was the ambiguity on whether it was ok to drop the bond lists in the group. Our decoder was not ok with it (it is now), but one may argue that we were too restrictive in interpreting the 1.0 spec. In short: all good for me...
Basically, if no one objects, I will merge those changes into master.
* Support for binary strategy 16 (Run-length encoded 8-bit array) * Added bondResonanceList field (global and per-group) * Made groupType.bondAtomList & groupType.bondOrderList optional * Updated version string in StructureData * Extended unit tests for bonds (incl. new test MMTF files with resonance fields) Note that we can write files that our 1.0 decoder cannot read (although the specs were ambiguous there) when having groups without bonds or otherwise empty groupType.bondAtomList or groupType.bondOrderList. Those fields were expected to exist in any MMTF file, while now they are optional and will not be written unless there is data in them.
This will set us up to be able to handle rcsb spec v 1.1.
This isn't ready yet:
todolist:
- [ ] test for transforms (BioAssembly wasn't a vector of vectors)- [ ] tests for output of to_string methodsI haven't tested if this works yet. just made a nice starting point.
solves #19