rcsb/mmtf-c

The c/c++ implementation of the MMTF API, decoder and encoder.

mmtf-c's Issues

bondOrderList/bondAtomList not required

Ran into this with pymol, getting segfaults if a group has no bondAtomList or bondOrderList. (HOH, O only group)

according to the 'new' spec mmtf should be able to handle groups that don't have the keys

bondAtomList or bondOrderList, or bondResonanceList

I'm not sure how much you want mmtf-c to support of the new spec... but it would be nice :)

Decoder API: hide functions that should not be "public"

The header file (mmtf_parser.h) defines several functions that should not be "public". Those could be moved to the C file.

Functions that should remain public:

• MMTF_parser_MMTF_container_new();
• MMTF_parser_parse_msgpack(st, st_len, container);
• MMTF_parser_MMTF_container_destroy(container);

ncsOperatorList field is missing

The decoder doesn't handle the ncsOperatorList field at the moment, see the spec

Error in parsing float values with msgpack-c 2.1.1

When loading most of the test data (e.g. "./compile_and_run.sh data/173D.mmtf"), I get errors "Error in MMTF_parser_fetch_float: the entry encoded in the MMTF is not a float."

This happens when parsing the "resolution" and "rWork" fields of the mmtf.
It turns out that the "FLOAT" type is a 64-bit float and the C version of msgpack determines that the variable is a 32-bit float. Interestingly enough, the C++ version of msgpack (enabled here by defining "MMTF_MSGPACK_USE_CPP11") determined that the variables are 64-bit floats (hence no error there).

Given that I am not sure that we have full control over whether msgpack will use 32 or 64 bits for Float type variables (and they are stored in the "via" variable as double anyways), we should probably accept both. Hence, my proposed change is as follows:

float MMTF_parser_fetch_float(const msgpack_object* object) {
-    if (object->type != MMTF_MSGPACK_TYPE(FLOAT)) {
+    if (   object->type != MMTF_MSGPACK_TYPE(FLOAT32)
+        && object->type != MMTF_MSGPACK_TYPE(FLOAT64)) {
         fprintf(stderr, "Error in %s: the entry encoded in the MMTF is not a float.\n", __FUNCTION__);
         return NAN;
     }

Given the changelogs from msgpack-c, this might only work for versions 2.1.0 or newer, but they are deprecating the use of the "FLOAT" and "DOUBLE" types, so we should probably update too.
Alternatively, one may drop the check altogether and just blindly interpret the input as a floating point number...

Unable to decode any MMTF file

Error in MMTF_unpack_from_msgpack_object: the entry encoded in the MMTF is not a map is output from all the demo executables when provided with a valid MMTF file. The build environment is Ubuntu 16.04 x86-64, using the latest versions of MessagePack and GCC toolkit. Attempts to use older versions of MessagePack as well as alternate C++ compilers made no difference.

Decoder API: adopt a better naming convention

Adopt a better naming convention for functions based on the operation. Currently the "_destroy" functions free memory. Better names will be:
"_destroy" -> "_free"
"_destroy_inside" -> "_destroy"

code documentation vs. MMTF spec

I like to raise a point for discussion.

Most of the (doxygen) code documentation describes the MMTF data model. In my opinion this is redundant with the MMTF spec and not necessary. I would expect that the C implementation is as close to the spec as possible, and I would read the spec rather than the doxygen documentation when working with the library. That basically means that e.g. the struct members in mmtf_parser.h don't need any documentation.

On the other hand, anything that is implementation specific needs clean doxygen documentation, of course. E.g. all public functions in mmtf_parser.h.

why uint64_t for array lengths?

Question: Why "uint64_t" and not "size_t" for all the array lengths ("Count" members in structs and "length" argument in fetch functions). If there is no particular reason, I suggest to change those to "size_t".

Decoder API: reconsider function names

Remove "MMTF_parser_" from function name if the operation is not specific to parsing. E.g. "MMTF_parser_MMTF_container_destroy" to "MMTF_container_destroy".

Spec

https://github.com/rcsb/mmtf

replace tabs with spaces and delete stale debug code

This is cosmetic, but should be done before the official release.

Could possibly use clang-format for this.