Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add json outputter for debugging #34

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

danpf
Copy link
Collaborator

@danpf danpf commented Aug 16, 2019

I think this is reasonable to have.... I've had some interest myself in better mmtf 'diff' tools, and had a complaint or two from rosetta users about diffing abilities.

let me know what you think!

i can add an app to do this too if that's of interest

@speleo3
Copy link
Collaborator

speleo3 commented Aug 16, 2019

insCodeList and altLocList strings get dumped as null bytes: "\u0000". Would look better as empty string, right?

@gtauriello
Copy link
Collaborator

gtauriello commented Aug 16, 2019

Some comments and observations:

  • It doesn't seem to be necessary to convert int8 to int32. Actually (due to a typo I guess), the bondResonanceList field is not converted for the json output. On my machine I could safely drop those conversions for bondOrderList, bondResonanceList and secStructList. Any reason to keep them?
  • The extra properties cannot be dumped into valid JSON if they contain encoded data. I tested it with the attached MMTF file (which is the file generated in the "extra data fields" test case of mmtf_tests.cpp). At least Python and Sublime complained about the generated JSON file. So we should either drop the json-dump for the extra properties or just dump the keys.
  • The traverse.cpp example code had a json dumper which I quickly extended to compare with the to_json function. The advantage in traverse.cpp is that it uses null for data which wasn't set. This can be confusing otherwise for optional numeric fields (e.g. resolution) which are set to std::numeric_limits<T>::max() if they are not provided in the input MMTF file. Also traverse.cpp does some custom pretty-printing suited for MMTF-content. On the down-side, the traverse.cpp is an ugly C-style C++ code (my apologies since I wrote it) which was originally meant to allow diffing with the mmtf-c library and grew beyond sustainability.
  • I also included the to_json function in traverse.cpp to be able to call it from there.
  • I don't think that the to_json.cpp app is useful within tests and I would drop it. The code is compiled anyway, so a pure "it compiles" test is not adding anything. It could be useful in examples but the traverse.cpp could cover that (btw: you can also call the StructureData::print functionality from traverse.cpp).

So to sum up: If we have such a to_json function as part of the library it should handle unset optional fields. I suggest to drop them since this already happens within GroupType objects (handled automatically via msgpack_encoders.hpp). Also the items above (unneeded conversions, faulty JSON with extra props, unneeded to_json.cpp) should be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants