Converters API #2882

cbouy · 2020-07-30T23:54:56Z

Changes made in this Pull Request:

Makes u.atoms.convert_to("PACKAGE") case insensitive
Pass kwargs to converters: u.atoms.convert_to("rdkit", NoImplicit=False)
Adds u.atoms.convert_to.package() methods automatically from the metaclass magic, with TAB completion support, and docstring from the converter

The same logic can be applied to writers if that's needed

The tests will fail for now as I have written them with #2775 in mind.

Also, I'm not sure if I've put the code in the right place and if the names I've come up with are relevant.

I'm not sure how this will work out with Sphinx though, since all the convert_to.package() methods are made automatically through setattr. Similar concern for convert_to in AtomGroup, is there a way to tell Sphinx to show a particular docstring instead of considering it like an attribute, and if yes, which docstring should it show ?

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

pep8speaks · 2020-07-30T23:55:03Z

Hello @cbouy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file package/MDAnalysis/__init__.py:

Line 211:1: E402 module level import not at top of file

In the file package/MDAnalysis/converters/OpenMMParser.py:

Line 31:80: E501 line too long (146 > 79 characters)

In the file package/MDAnalysis/converters/ParmEd.py:

Line 27:80: E501 line too long (113 > 79 characters)
Line 61:80: E501 line too long (88 > 79 characters)
Line 76:80: E501 line too long (94 > 79 characters)
Line 145:80: E501 line too long (95 > 79 characters)
Line 166:80: E501 line too long (95 > 79 characters)
Line 225:80: E501 line too long (86 > 79 characters)
Line 245:80: E501 line too long (83 > 79 characters)
Line 259:80: E501 line too long (85 > 79 characters)
Line 274:13: E731 do not assign a lambda expression, use a def
Line 334:80: E501 line too long (109 > 79 characters)
Line 335:80: E501 line too long (84 > 79 characters)
Line 336:80: E501 line too long (99 > 79 characters)

In the file package/MDAnalysis/converters/ParmEdParser.py:

Line 44:80: E501 line too long (85 > 79 characters)
Line 169:9: E266 too many leading '#' for block comment
Line 276:9: E266 too many leading '#' for block comment

In the file package/MDAnalysis/core/accessors.py:

Line 27:80: E501 line too long (100 > 79 characters)
Line 28:80: E501 line too long (80 > 79 characters)
Line 139:80: E501 line too long (91 > 79 characters)

In the file package/MDAnalysis/topology/__init__.py:

Line 311:28: E231 missing whitespace after ','

Comment last updated at 2021-05-10 18:20:47 UTC

cbouy · 2020-07-31T00:02:23Z

Forgot to tag @MDAnalysis/coredevs !

orbeckst

Your pandas plot-style interface is a good middle-ground. I don't know how to solve the problem with the docs. Perhaps look into how pandas documents DataFrame.plot.

I suggest you start building converter things in MDAnalysis.converters although others might have different opinions.

package/MDAnalysis/lib/accessors.py

package/MDAnalysis/core/groups.py

…or and the converter module instances

codecov · 2021-04-25T00:25:00Z

Codecov Report

Merging #2882 (26ebd6e) into develop (d734a89) will decrease coverage by 0.00%.
The diff coverage is 96.14%.

@@             Coverage Diff             @@
##           develop    #2882      +/-   ##
===========================================
- Coverage    93.56%   93.56%   -0.01%     
===========================================
  Files          172      176       +4     
  Lines        22785    22823      +38     
  Branches      3191     3193       +2     
===========================================
+ Hits         21319    21354      +35     
- Misses        1416     1419       +3     
  Partials        50       50

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/__init__.py	`100.00% <ø> (ø)`
package/MDAnalysis/topology/__init__.py	`100.00% <ø> (ø)`
package/MDAnalysis/converters/ParmEd.py	`93.18% <93.18%> (ø)`
package/MDAnalysis/converters/ParmEdParser.py	`98.44% <98.44%> (ø)`
package/MDAnalysis/__init__.py	`92.10% <100.00%> (+0.21%)`	⬆️
package/MDAnalysis/converters/OpenMM.py	`97.05% <100.00%> (ø)`
package/MDAnalysis/converters/OpenMMParser.py	`100.00% <100.00%> (ø)`
package/MDAnalysis/converters/RDKit.py	`97.31% <100.00%> (ø)`
package/MDAnalysis/converters/RDKitParser.py	`96.72% <100.00%> (ø)`
package/MDAnalysis/converters/__init__.py	`100.00% <100.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d734a89...26ebd6e. Read the comment docs.

lilyminium · 2021-04-26T19:57:48Z

@cbouy please just let us know when this is ready for review :-)

cbouy · 2021-04-29T09:08:59Z

I still need to write the tests but most of the code and docs should be ready for review

richardjgowers · 2021-05-04T21:42:32Z

I'm not really convinced that we need convert_to.rdkit() rather than convert_to('rdkit'). The syntax of convert_to.X doesn't feel hugely intuitive to me, I can't think of a similar case? The selling point seems to be tab completion, could we not instead do something smart with the docstrings of convert_to? being magically patched? If we split out the case insensitivity we can merge that as is.

orbeckst · 2021-05-04T22:22:28Z

The syntax of convert_to.X doesn't feel hugely intuitive to me, I can't think of a similar case?

There was a fairly long discussion on this and we found that pandas DataFrames do something similar with their different plots (see #2790 (comment) and thereabouts). The majority seemed to favor TAB-discoverability.

richardjgowers

Ok lgtm

cbouy · 2021-05-07T15:28:41Z

I've included the OpenMM converter as well (just noticed it was there), and hopefully fixed #3262 in the process

jbarnoud · 2021-05-07T15:31:51Z

I've included the OpenMM converter as well (just noticed it was there), and hopefully fixed #3262 in the process

Make sure you also move the documentation stubs.

lilyminium · 2021-05-08T19:44:30Z

@cbouy could you change the np.int in your tests to int?

Edit: Ah, you already test that. Could you explain why you're also testing np.int?

lilyminium · 2021-05-09T17:52:25Z

@cbouy could you please rebase on develop and see if that fixes the tests? :)

IAlibay · 2021-05-09T18:19:53Z

Sorry @cbouy in the interest of getting this merged faster & not forcing you to work on a Sunday I went ahead and did the update against develop.

IAlibay

Went ahead and fixed the doc issues.

IAlibay · 2021-05-09T21:18:58Z

package/MDAnalysis/converters/OpenMMParser.py


 .. versionadded:: 2.0.0


 Converts an
-`OpenMM <http://docs.openmm.org/latest/api-python/generated/simtk.openmm.app.topology.Topology.html#simtk.openmm.app.topology.Topology>`_
+`OpenMM topology <http://docs.openmm.org/latest/api-python/generated/simtk.openmm.app.topology.Topology.html#simtk.openmm.app.topology.Topology>`_


sphinx was complaining about doubling of OpenMM link definition, so I renamed the topology as such.

IAlibay · 2021-05-09T21:19:51Z

package/MDAnalysis/converters/RDKitParser.py

@@ -22,15 +22,15 @@
 #

 """
-RDKit topology parser
-=====================
+RDKit topology parser --- :mod:`MDAnalysis.converters.RDKitParser`


Added these :mod: entries since they were present in the non Parser modules.

IAlibay · 2021-05-09T21:20:36Z

package/doc/sphinx/source/documentation_pages/converters.rst


 .. rubric:: Available converters

 .. toctree::
   :maxdepth: 1

-   converters/ParmEdParser
-   converters/RDKitParser
+   converters/init


sphinx was complaining about the presence of init.rst without a toctree entry for it. I added it, although it isn't super informative as an entry, should we just remove it?

Alternatively, we could move the converters.rst contents into converters.__init__ ?

Went with the path of least resistance here and just moved a tiny bit of init's doc to the converters.rst. We can always clean up in 2.1.0 (or post beta) if it's really worth it.

package/MDAnalysis/coordinates/ParmEd.py

IAlibay · 2021-05-09T21:54:13Z

Sorry again for hijaking the PR @cbouy, just added a couple of small commits (docs + tests) to see if we can get it merged today.

IAlibay · 2021-05-09T22:08:32Z

This is concerning, the 3.6 / numpy 1.16 run failed twice with the same error... (just restarted things to see if it'll do it a third time in a row)

[gw1] linux -- Python 3.6.13 /usr/share/miniconda/envs/test/bin/python

self = <test_rdkit.TestRDKitConverter object at 0x7f3a1297ceb8>, smi = '[He]'

    @pytest.mark.parametrize("smi", ["[H]", "C", "O", "[He]"])
    def test_single_atom_mol(self, smi):
        u = mda.Universe.from_smiles(smi, addHs=False,
                                     generate_coordinates=False)
        mol = u.atoms.convert_to("RDKIT")
        assert mol.GetNumAtoms() == 1
>       assert mol.GetAtomWithIdx(0).GetSymbol() == smi.strip("[]")
E       AssertionError: assert 'C' == 'He'
E         - He
E         + C

Ok, it's not done it again, it's probably the whole cache thing -- let's get this merged so we can get that one merged too.

edit: the failure rate for py3.6 numpy 1.16 is very high, I wonder if it's actually linked to rdkit 2020 vs 2021.

IAlibay · 2021-05-10T15:15:01Z

@lilyminium @orbeckst can we get a quick re-review with aim to merge please?

orbeckst

After a quick read through, this looks good. I would just add an explicit statement to CHANGELOG about the new converter module. I added a suggestion — @IAlibay @cbouy you can either apply the suggestion or proceed without it if you think that's more appropriate.

package/CHANGELOG

lilyminium

LGTM! Thanks @cbouy and @IAlibay for pushing it forward

IAlibay · 2021-05-10T19:07:45Z

Only failing check here is codecov (just an error cover, can deal with it some other time).

Thanks for your hard work here @cbouy 🎉 -- squash merging

orbeckst requested changes Jul 31, 2020

View reviewed changes

package/MDAnalysis/lib/accessors.py Outdated Show resolved Hide resolved

package/MDAnalysis/core/groups.py Show resolved Hide resolved

package/MDAnalysis/core/groups.py Outdated Show resolved Hide resolved

orbeckst added the GSoC GSoC project label Aug 7, 2020

orbeckst assigned fiona-naughton, IAlibay, orbeckst, lilyminium and jbarnoud Aug 7, 2020

cbouy mentioned this pull request Apr 22, 2021

Improving the RDKitConverter caching system #2942

Merged

4 tasks

IAlibay added this to the 2.0 milestone Apr 22, 2021

Cédric Bouysset added 7 commits April 24, 2021 18:55

kwargs for converter + case insensitive package

d10357d

fixes

515b108

prototype for convert_to accessor

95b768a

automatic addition of convert_to.lib() methods

840bfd9

doc + tests

db45e3e

move accessors

a29ee62

pep8

4ca5baf

cbouy force-pushed the converters-api branch from 43f6b34 to 4ca5baf Compare April 24, 2021 17:00

cbouy added 4 commits April 24, 2021 19:10

fix unused import

2161177

changed the Accessor and ConverterWrapper to cache the wrapped access…

e84273b

…or and the converter module instances

fix pep8

9d9586f

move tests

b93076a

richardjgowers marked this pull request as ready for review May 5, 2021 09:02

richardjgowers approved these changes May 5, 2021

View reviewed changes

cbouy added 4 commits May 7, 2021 17:08

move accessors to core

8456cf5

fix tests

18dc71e

include openmm

e5abca5

fix and move openmm tests

fa7077c

cbouy added 5 commits May 7, 2021 17:36

fix openmm docs

47b798e

fix imports

7f1fa38

fix docs

38ed979

fix openmm relative imports

646c479

fix tests

2f919cb

Merge branch 'develop' into converters-api

7db6ddf

Fix docs

249bc74

IAlibay reviewed May 9, 2021

View reviewed changes

Fix parmed imports, adds warning tests

5b25b49

IAlibay reviewed May 9, 2021

View reviewed changes

package/MDAnalysis/coordinates/ParmEd.py Show resolved Hide resolved

Remove init.rst from converters docs

8b6c18f

orbeckst approved these changes May 10, 2021

View reviewed changes

package/CHANGELOG Show resolved Hide resolved

lilyminium approved these changes May 10, 2021

View reviewed changes

Update changelog

26ebd6e

IAlibay merged commit cef7d3f into MDAnalysis:develop May 10, 2021

fiona-naughton added Component-Converters enhancement labels Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converters API #2882

Converters API #2882

cbouy commented Jul 30, 2020 •

edited

Loading

pep8speaks commented Jul 30, 2020 •

edited

Loading

cbouy commented Jul 31, 2020

orbeckst left a comment

codecov bot commented Apr 25, 2021 •

edited

Loading

lilyminium commented Apr 26, 2021

cbouy commented Apr 29, 2021

richardjgowers commented May 4, 2021

orbeckst commented May 4, 2021 •

edited

Loading

richardjgowers left a comment

cbouy commented May 7, 2021

jbarnoud commented May 7, 2021

lilyminium commented May 8, 2021 •

edited

Loading

lilyminium commented May 9, 2021

IAlibay commented May 9, 2021

IAlibay left a comment

IAlibay May 9, 2021

IAlibay May 9, 2021

IAlibay May 9, 2021

lilyminium May 9, 2021 •

edited

Loading

IAlibay May 10, 2021

IAlibay commented May 9, 2021

IAlibay commented May 9, 2021 •

edited

Loading

IAlibay commented May 10, 2021

orbeckst left a comment

lilyminium left a comment

IAlibay commented May 10, 2021

Converters API #2882

Converters API #2882

Conversation

cbouy commented Jul 30, 2020 • edited Loading

PR Checklist

pep8speaks commented Jul 30, 2020 • edited Loading

Comment last updated at 2021-05-10 18:20:47 UTC

cbouy commented Jul 31, 2020

orbeckst left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 25, 2021 • edited Loading

Codecov Report

lilyminium commented Apr 26, 2021

cbouy commented Apr 29, 2021

richardjgowers commented May 4, 2021

orbeckst commented May 4, 2021 • edited Loading

richardjgowers left a comment

Choose a reason for hiding this comment

cbouy commented May 7, 2021

jbarnoud commented May 7, 2021

lilyminium commented May 8, 2021 • edited Loading

lilyminium commented May 9, 2021

IAlibay commented May 9, 2021

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay May 9, 2021

Choose a reason for hiding this comment

IAlibay May 9, 2021

Choose a reason for hiding this comment

IAlibay May 9, 2021

Choose a reason for hiding this comment

lilyminium May 9, 2021 • edited Loading

Choose a reason for hiding this comment

IAlibay May 10, 2021

Choose a reason for hiding this comment

IAlibay commented May 9, 2021

IAlibay commented May 9, 2021 • edited Loading

IAlibay commented May 10, 2021

orbeckst left a comment

Choose a reason for hiding this comment

lilyminium left a comment

Choose a reason for hiding this comment

IAlibay commented May 10, 2021

cbouy commented Jul 30, 2020 •

edited

Loading

pep8speaks commented Jul 30, 2020 •

edited

Loading

codecov bot commented Apr 25, 2021 •

edited

Loading

orbeckst commented May 4, 2021 •

edited

Loading

lilyminium commented May 8, 2021 •

edited

Loading

lilyminium May 9, 2021 •

edited

Loading

IAlibay commented May 9, 2021 •

edited

Loading