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

Top ten reasons why you should be parsing PDB. #1441

Closed
wants to merge 3 commits into from

Conversation

hmenke
Copy link
Member

@hmenke hmenke commented Sep 26, 2017

import espressomd
system = espressomd.System()
system.part.(pdb_file="ortho_dimer.pdb", type=0, first_id=0)

Fixes #1438

@codecov
Copy link

codecov bot commented Sep 26, 2017

Codecov Report

Merging #1441 into python will increase coverage by 1.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           python    #1441      +/-   ##
==========================================
+ Coverage   39.58%   40.68%   +1.09%     
==========================================
  Files         364      372       +8     
  Lines       30508    31227     +719     
  Branches     5578     5697     +119     
==========================================
+ Hits        12078    12704     +626     
- Misses      18390    18411      +21     
- Partials       40      112      +72
Impacted Files Coverage Δ
src/core/PdbParser.hpp 20% <ø> (ø) ⬆️
src/core/PdbParser.cpp 1.07% <0%> (ø) ⬆️
src/core/readpdb.cpp 0% <0%> (ø) ⬆️
src/core/verlet.cpp 97.58% <0%> (-2.42%) ⬇️
src/core/forces_inline.hpp 48.28% <0%> (-0.51%) ⬇️
src/core/iccp3m.cpp 46.08% <0%> (-0.39%) ⬇️
src/core/energy_inline.hpp 29.1% <0%> (-0.32%) ⬇️
src/core/integrate.cpp 37.32% <0%> (-0.03%) ⬇️
src/core/observables/LbVelocityProfile.hpp 0% <0%> (ø) ⬆️
src/core/electrokinetics_pdb_parse.cpp 0% <0%> (ø)
... and 37 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 db8b559...a91dfd7. Read the comment docs.


check_type_or_throw_except(pdb_file , 1, str , "pdb_file must be string")
check_type_or_throw_except(itp_file , 1, str , "itp_file must be string")
check_type_or_throw_except(type , 1, int , "type must be an integer")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is a bad name for a Python variable as it's a keyword. You can overwrite the keyword like you've done, but that can be confusing at times.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I didn't want to break compatibility in terms of naming. You can also easily restore the original meaning by simply doing del type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to be consistent, right?

# Particle Type
property type:
"""
The type id of the Particle.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a new function? So you can choose the signature freely without compatibility concerns. I assume you did this to mimic the signature of system.particle.add -- though that does not have the overwriting problem because it only accepts a type argument through **kwargs.

Copy link
Member

@mkuron mkuron Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Properties named type don't have that problem because they don't pollute the local namespace with a type variable. They are only accessible through self.type. So if you want to name an rgument type for consistency, you should go through **kwargs.

@fweik
Copy link
Contributor

fweik commented Sep 26, 2017

The python interface looks good to me. This should have a test, and it should be referenced in the io section of the docs, so that people can actually find it. In general I' not so sure if we should implement that ourself, as pdb parser are available e.g. from MDAnalysis. (The pdb parser in Espresso can also (partly) parse gromacs force fields, which there are also python packages for, e.g. ACPYPE).

On a more general note, I think the IO stuff should be implemented as a class hierarchy in c++,
which would make it e.g. possible to use the same unit tests and script interface for all methods.
This was something @KaiSzuttor was thinking about doing, but I don't know that the status on
that is. There is a couple of other io methods that could be integrated into that too (vtf, vtk, ...)

@KaiSzuttor
Copy link
Member

I haven't done anything on that yet, but I also like the idea of not reinventing the wheel and use external tools if available.

@fweik fweik added this to the Espresso 4.1 milestone Jul 13, 2018
@KaiSzuttor KaiSzuttor removed this from the Espresso 4.1 milestone Jul 15, 2019
@RudolfWeeber
Copy link
Contributor

Do we still aim to re-activate the PDB-support in the Espresso core, or do we fully rely on the md analysis bindings?
@fweik, @jngrad?

@jngrad
Copy link
Member

jngrad commented Oct 14, 2019

I wrote a PDB parser during my PhD. Parsing my own PDB files was easy. The trouble started when other users tried using my tool with their PDB files. I spent countless hours improving the code to read custom PDB formats used by various modeling softwares. A lot of time also went into tech support, providing scripts to post-process PDB files with regexes (simple cases) or Python (desperate cases) and explaining the history behind specific details in the PDB standard (e.g. the iCode field). I'm not sure providing a PDB parser in ESPResSo is worth the inevitable tech support on the mailing list. We could instead use well-established tools like MDAnalysis or Biopython. The C++ PdbParser class also seems to expect whitespace-separated values, which is not guaranteed in the PDB standard, meaning some users will have to use MDAnalysis or Biopython anyway to strip extra data and transform HETATM into ATOM lines to comply to the "ESPResSo PDB" format. Konrad and I once proposed to have a tutorial explaining the general procedure to load a topology in ESPResSo, using the Gromacs .gro/.itp as example, but we couldn't find the time to actually do it.

@fweik
Copy link
Contributor

fweik commented Oct 14, 2019

I did write the the PDB parser and I agree with @jngrad.

@RudolfWeeber
Copy link
Contributor

So, the conclusion from @jngrad and @fweik's comments is to remove the pdb parser from the core and close this PR, correct?

@jngrad
Copy link
Member

jngrad commented Oct 16, 2019

Yes. I will take care of removing the current parser code from the python branch.

@jngrad jngrad closed this Oct 16, 2019
bors bot added a commit that referenced this pull request Oct 17, 2019
3252: Factor out ParticleList r=jngrad a=fweik

Follow up on #3251.

Description of changes:
 - Pulling `ParticleList` out of `particle_data.hpp` to get better
   header disentanglement.


3256: Remove tutorial 10 and unused LaTeX files r=fweik a=jngrad

Closes #3211

Description of changes:
- removed tutorial 10
- removed unused LaTeX preambles

3257: Remove PDB parser feature r=fweik a=jngrad

The consensus offline at the ICP and online in #1441 is to drop support of the PDB parser feature in favor of the dedicated python package MDAnalysis.

Co-authored-by: Florian Weik <[email protected]>
Co-authored-by: Jean-Noël Grad <[email protected]>
bors bot added a commit that referenced this pull request Oct 17, 2019
3252: Factor out ParticleList r=jngrad a=fweik

Follow up on #3251.

Description of changes:
 - Pulling `ParticleList` out of `particle_data.hpp` to get better
   header disentanglement.


3256: Remove tutorial 10 and unused LaTeX files r=fweik a=jngrad

Closes #3211

Description of changes:
- removed tutorial 10
- removed unused LaTeX preambles

3257: Remove PDB parser feature r=fweik a=jngrad

The consensus offline at the ICP and online in #1441 is to drop support of the PDB parser feature in favor of the dedicated python package MDAnalysis.

Co-authored-by: Florian Weik <[email protected]>
Co-authored-by: Jean-Noël Grad <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python support for pdb parser
6 participants