mittinatten / freesasa

C-library for calculating Solvent Accessible Surface Areas
http://freesasa.github.io/
MIT License
105 stars 37 forks source link

Support CIF #51

Closed mittinatten closed 3 years ago

mittinatten commented 3 years ago

Related to #48. Using gemmi (https://gemmi.readthedocs.io/en/latest/index.html).

Support all CLI options also for CIF

Testing

mittinatten commented 3 years ago

History is a bit messy because I moved some changes that were due to changing editor setup to the dev branch in order to keep the diff here only related to the new feature.

mittinatten commented 3 years ago

Did some rebasing to make the commits clearer

danny305 commented 3 years ago

Super excited! Let me know if you encounter any bugs you need me to address!

mittinatten commented 3 years ago

So I'm going through everything one last time, and have committed a few minor things. A few things are stylistic. And I found a better solution for handling output. There is an option --output to specify another file than stdout. So I pass a filename instead of a file pointer to the CIF output function, see d609021.

I noticed we are missing docs for CIF output, so I'll fix that too.

danny305 commented 3 years ago

Awesome! yeah I originally wrote it to output to a file so I am glad you added that additional capability back.

mittinatten commented 3 years ago

I made one other minor change. RSA-values are output regardless of FREESASA_OUTPUT_SKIP_REL, in the case where there is no reference available the output will then look like this, which I think is still useful.

loop_
_freeSASA_rsa.asym_id
_freeSASA_rsa.seq_id
_freeSASA_rsa.comp_id
_freeSASA_rsa.abs_total
_freeSASA_rsa.rel_total
_freeSASA_rsa.abs_side_chain
_freeSASA_rsa.rel_side_chain
_freeSASA_rsa.abs_main_chain
_freeSASA_rsa.rel_main_chain
_freeSASA_rsa.abs_apolar
_freeSASA_rsa.rel_apolar
_freeSASA_rsa.abs_polar
_freeSASA_rsa.rel_polar
A 1 MET 54.393508 ? 19.085046 ? 35.308462 ? 28.483151 ? 25.910357 ?
A 2 GLN 74.212106 ? 70.132368 ? 4.079738 ? 11.657433 ? 62.554673 ?
...
danny305 commented 3 years ago

Yeah I made it output RSA regardless bc why not. Its a separate block if you don't need it then ignore it. If values are missing then they will be either ? or .. Which is standard for cif.

I don't see the change you made?

mittinatten commented 3 years ago

See c204ce7b9c6c11

mittinatten commented 3 years ago

Yeah I thought about that too. I think the docs probably makes the most sense, otherwise we would need to add it to all the fields, I assume: _freeSASA_rsa.abs_side_chain_AA2 or _freeSASA_rsa.abs_side_chain_Angstrom^2, or something, which looks kind of strange.

danny305 commented 3 years ago

Yeah it looks super clunky. I think the docs is a better place.

mittinatten commented 3 years ago

I'm done for the day, but think we're really close now!

mittinatten commented 3 years ago

One more thing, if you can think of any CLI tests that we can use to check that everything works and catch regressions later that would be great. At least we should have one simple case where we compare with a stored CIF output so that we get an error if future modifications changes the format by accident.

danny305 commented 3 years ago

Im not sure I understand? Gemmi writes out the format and enforces it? if we did not follow the cif format then gemmi would error out? Could you elaborate/clarify exactly the kind of test you want?

mittinatten commented 3 years ago

I was simply thinking of one or more test that runs through the different branches of the CIF output code, so that if something breaks we catch it automatically. For instance if the freesasa_node interface changes somehow in the future, the test suite should trigger an error also for CIF output, to avoid regressions. Also we should check that illegal CLI options are caught with an error message, i.e. --format=cif 1234.pdb and --format=pdb --cif, etc.

mittinatten commented 3 years ago

I added a few, that's probably enough for now.

mittinatten commented 3 years ago

I've been through the code enough times now to be pretty sure this is ok to publish. Thanks so much for your effort, really nice to collaborate!

I will merge it to the dev-branch and add some version information, etc, before merging to master. As mentioned I will release it as 2.1-beta to allow it to mature a little bit more. That means anyone who clones the repository will get the new features, but they won't be in the published docs yet, and we are still free to make minor changes to for example CLI options, etc, if we realize something is inconsistent in the interface.