export_helpers: BondAdder, compressGroupList

rcsb / mmtf-cpp

The pure C++ implementation of the MMTF API, decoder and encoder.

MIT License

21 stars 24 forks source link

export_helpers: BondAdder, compressGroupList #11

Closed speleo3 closed 5 years ago

speleo3 commented 6 years ago

This pull request adds two helpers for molecule export from tabular data:

mmtf::BondAdder for efficient bond adding
mmtf::compressGroupList for creating a non-redundant groupList

We implemented those for MMTF export in PyMOL, but since the code is not PyMOL specific, we'd like to contribute it directly to the mmtf-cpp library.

See examples/tableexport.cpp for an example how to use these helpers.

gtauriello commented 6 years ago

This should wait quickly until we reach a decision in #11, on whether we drop C++-03 support.

Also two quick comments:

Is "pragma once" a standard way to avoid multi-include? I somehow thought it was compiler-dependent, but if it is generally acceptable, I am ok with it. Always preferred it over the old-school defines for my own non-portable projects.
Do the CI tests compile the examples? Just wondering if it makes sense to add it there.

speleo3 commented 6 years ago

#pragma once: "non-standard but widely supported" (Wikipedia). I'll replace it with include-guards to be consistent with your other files.

I haven't hooked up CI for the example or added any tests to the suite.

gtauriello commented 5 years ago

Seems like the test is struggling in Windows. Fails at line 510 (REQUIRE_THROWS_AS(mmtf::BondAdder(sd), mmtf::EncodeError);) in the new TEST_CASE("Test export_helpers"). @frodofine @danpf @speleo3 Does one of you have an active development setup in Windows? Tough for me to debug that one as I only have Linux and Mac setups...

speleo3 commented 5 years ago

@gtauriello - replacing emplace_back by push_back should do the trick. Not sure why the log reports line 510.

gtauriello commented 5 years ago

I see. Probably it reports the last Catch2-check that was performed then. Seems to be fixed now yes.

speleo3 commented 5 years ago

by the way, I'm a big fan of git merge --squash, so feel free to use the "squash and merge" option once this PR is accepted.

gtauriello commented 5 years ago

I'm generally not a fan of "rewriting history" so I usually avoid squashing, but I understand the reasons in favor of it. Seems a bit of a symptom of how github displays the commit history. Would be so easy for them to just add a filter to only see merge-commits and expand if desired (really easy to do on command line btw). That being said: I guess I could try squashing here for this project and see how it works in practice...

gtauriello commented 5 years ago

@speleo3 sorry for the delay on this. I didn't find time to look at it this week. I will do it asap.

speleo3 commented 5 years ago

thanks @gtauriello !

speleo3 commented 5 years ago

FYI, we're using this with PyMOL now: https://github.com/schrodinger/pymol-open-source/commit/be7a8d1f49a0230e81d24cd2193723305429965c