Update to chemfiles 0.10 #3126

Luthaf · 2021-02-19T10:59:38Z

Changes made in this Pull Request:

Chemfiles coordinate reader updated to the API changes in chemfiles 0.10 (in practice, the only relevant change here is a different constructor for UnitCell).

Chemfiles 0.10 contains multiple fixes to different format readers; and add support for crystalographic CIF files, and extended XYZ (cf https://wiki.fysik.dtu.dk/ase/ase/io/formatoptions.html#extxyz)

PR Checklist

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

pep8speaks · 2021-02-19T10:59:41Z

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

In the file package/MDAnalysis/coordinates/chemfiles.py:

Line 26:80: E501 line too long (83 > 79 characters)
Line 28:80: E501 line too long (84 > 79 characters)
Line 40:80: E501 line too long (85 > 79 characters)
Line 132:80: E501 line too long (85 > 79 characters)
Line 169:80: E501 line too long (80 > 79 characters)
Line 316:80: E501 line too long (80 > 79 characters)
Line 346:80: E501 line too long (80 > 79 characters)

In the file testsuite/MDAnalysisTests/coordinates/test_chemfiles.py:

Line 26:80: E501 line too long (83 > 79 characters)
Line 27:80: E501 line too long (86 > 79 characters)
Line 32:80: E501 line too long (89 > 79 characters)
Line 33:80: E501 line too long (91 > 79 characters)
Line 40:80: E501 line too long (85 > 79 characters)
Line 47:80: E501 line too long (136 > 79 characters)
Line 49:80: E501 line too long (89 > 79 characters)
Line 52:80: E501 line too long (83 > 79 characters)
Line 132:80: E501 line too long (85 > 79 characters)

Comment last updated at 2021-05-08 00:15:17 UTC

codecov · 2021-02-19T11:16:41Z

Codecov Report

Merging #3126 (b836d43) into develop (da46e84) will increase coverage by 0.52%.
The diff coverage is 94.59%.

@@             Coverage Diff             @@
##           develop    #3126      +/-   ##
===========================================
+ Coverage    93.03%   93.55%   +0.52%     
===========================================
  Files          172      172              
  Lines        22724    22806      +82     
  Branches      3193     3193              
===========================================
+ Hits         21141    21337     +196     
+ Misses        1533     1419     -114     
  Partials        50       50

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/chemfiles.py	`96.75% <94.59%> (+64.93%)`	⬆️
package/MDAnalysis/analysis/base.py	`96.61% <0.00%> (-0.03%)`	⬇️
package/MDAnalysis/analysis/diffusionmap.py	`100.00% <0.00%> (ø)`
package/MDAnalysis/analysis/helix_analysis.py	`100.00% <0.00%> (ø)`
...nalysis/analysis/hydrogenbonds/wbridge_analysis.py	`97.42% <0.00%> (+<0.01%)`	⬆️
...DAnalysis/analysis/hydrogenbonds/hbond_analysis.py	`98.70% <0.00%> (+0.06%)`	⬆️
package/MDAnalysis/analysis/align.py	`98.32% <0.00%> (+0.12%)`	⬆️

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 da46e84...b836d43. Read the comment docs.

richardjgowers · 2021-02-19T15:06:17Z

package/MDAnalysis/coordinates/chemfiles.py

@@ -333,7 +347,12 @@ def _timestep_to_chemfiles(self, ts):
        if ts.has_velocities:
            frame.add_velocities()
            frame.velocities[:] = ts.velocities[:]
-        frame.cell = chemfiles.UnitCell(*ts.dimensions)
+


If this is the only real change, could we have a try/except here and allow 0.9 and 0.10?

This is the only change in the part of the API used in MDA yes.

I could add a version check here, but for the future I'd like to not keep old versions around too long, and drop 0.9 when 0.11 comes out 😃

Either way this goes, can the minimum version be pinned in the gh actions CI workflow? That way we can be sure we're picking up the right thing.

Luthaf · 2021-02-28T14:17:58Z

@IAlibay

Either way this goes, can the minimum version be pinned in the gh actions CI workflow? That way we can be sure we're picking up the right thing.

I've pinned chemfiles to == 0.9 in CI, do you think there should be automated tests with both 0.9.3 and 0.10.x?

IAlibay · 2021-02-28T14:28:14Z

@IAlibay

Either way this goes, can the minimum version be pinned in the gh actions CI workflow? That way we can be sure we're picking up the right thing.

I've pinned chemfiles to == 0.9 in CI, do you think there should be automated tests with both 0.9.3 and 0.10.x?

Probably the best thing here is to do >=0.9 and then if we want to test legacy versions we can create a cron job for it when we get around to implementing #3052 (at least in my opinion, ideally the only legacy version we really want to test at every push is numpy, otherwise we'll quickly end up with a giant matrix).

Luthaf · 2021-03-03T17:57:54Z

The codecov failure is weird, but other than that this should be ready to go!

IAlibay · 2021-03-13T02:47:07Z

@Luthaf Apologies for the delay. I thought it was probably easier to merge #3128 first so that any changes could then be updated to reflect those being done here (e.g. version of the docs being linked to). Could you resolve the conflicts and then we can aim for a merge?

Luthaf · 2021-03-13T15:22:23Z

Rebased! Let's see what CI says.

Luthaf · 2021-03-13T16:03:57Z

The python 3.6 runner fails with OSError: libssl.so.1.0.0: cannot open shared object file: No such file or directory when trying to load libchemfiles.so. Is there any difference in the environment between the 3.6 builders and the other ones? I'll investigate a bit more where the dependency on libssl comes from, since chemfiles itself is not using it.

IAlibay · 2021-03-13T17:02:01Z

The python 3.6 runner fails with OSError: libssl.so.1.0.0: cannot open shared object file: No such file or directory when trying to load libchemfiles.so. Is there any difference in the environment between the 3.6 builders and the other ones? I'll investigate a bit more where the dependency on libssl comes from, since chemfiles itself is not using it.

How strange. We've been having some occasional issues with gh actions and py3.6 lately. I have restarted CI to see if somehow that fixes it.

IAlibay · 2021-03-13T17:50:16Z

It seems like a consistent error. Strangely enough it's not happening on the 3.6 runner with numpy 1.16.0.
I guess there might be some kind of dependency clash here that is causing chemfiles to fail?

Luthaf · 2021-04-04T22:58:17Z

I think I found the origin of the issue here: libchemfiles links to libnetcdf, which links to libcurl. For a yet unknown reason, the conda build pulls in libssh2 from the biobuilds channel, which is linked by libcurl. libssh2 links to libopenssl.so.1.0.0, which is not installed (instead conda-forge gives us libopenssl.so.1.1.1). Given that the last update to libssh2 in biobuilds is more than 3 years old, I think it is best to stick with the conda-forge version.

It looks like the biobuilds channel is used only for clustalw, is this right? It looks like removing biobuilds from the main list of channels and instead using the channel as a one shot (conda install -c biobuilds clustalw=2.1) fixes the issue, and conda picks the newer llibssh2/openssl. I'll re-run the full CI with this configuration.

.github/workflows/gh-ci.yaml

Luthaf · 2021-04-05T12:51:18Z

The failed build is due to an HTTP error while fetching conda package. Could you restart it?

Luthaf · 2021-04-05T19:37:47Z

Everything looks ok now, thanks for the restart! I'm not sure what to do about the codecov patch coverage error: most of the "not covered" lines are related to codepath for chemfiles 0.9

IAlibay

Thanks @Luthaf sorry about all the CI related queries 😅 couple more things

.github/workflows/gh-ci.yaml

package/MDAnalysis/coordinates/chemfiles.py

IAlibay

@Luthaf I'll approve as-is, if you can add the suggested test that'd be great, if it proves more complicated than just that addition then let's just go with this and we can fix coverage some other time.

testsuite/MDAnalysisTests/coordinates/test_chemfiles.py

Otherwise conda picks outdated version of libssh2 from biobuilds, which links to non-existing libopenssl.so.1.0.0

IAlibay · 2021-05-07T10:18:27Z

package/MDAnalysis/coordinates/chemfiles.py

+
+        lengths = ts.dimensions[:3]
+        angles = ts.dimensions[3:]
+        if angles[0] == 0.0 and angles[1] == 0.0 and angles[2] == 0.0:


@Luthaf I'm actually not sure what's being done here, do you have a quick explanation you can provide?

This might be me being overly defensive. Does MDA guarantee angles to have valid values? If so this check can probably be removed.

O I see I guess you're capturing the [0., 0., 0., 0., 0., 0.] default case here mostly? (i.e. #2698)

I'm going to ping @richardjgowers here who was working on this for 2.0 if I remember correctly, and might have better insights.

Maybe the best thing to do here is just to check for [0., 0., 0., 0., 0., 0.] or None as the default box case, and then if that's not the case crash out?

Yes, chemfiles is fine with lengths of 0 but requires angles to be 90 in this case. I try to be generous in what code accept, so checking only for angles gives the maximal flexibility, but I'm also fine with checking if ts.dimensions is [0., 0., 0., 0., 0., 0.] and rejecting more wrong/missing cells.

So what's the decision here? Should I change this to check for either [0., 0., 0., 0., 0., 0.] or None?

richardjgowers · 2021-05-07T16:35:04Z

I’m fixing this behaviour in general in another PR, so either is fine because it’ll get changed later.

…

On Fri, May 7, 2021 at 17:19, Guillaume Fraux ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In package/MDAnalysis/coordinates/chemfiles.py <#3126 (comment)> : > @@ -375,7 +391,17 @@ def _timestep_to_chemfiles(self, ts): if ts.has_velocities: frame.add_velocities() frame.velocities[:] = ts.velocities[:] - frame.cell = chemfiles.UnitCell(*ts.dimensions) + + lengths = ts.dimensions[:3] + angles = ts.dimensions[3:] + if angles[0] == 0.0 and angles[1] == 0.0 and angles[2] == 0.0: So what's the decision here? Should I change this to check for either [0., 0., 0., 0., 0., 0.] or None? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3126 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGSGB7GYOKT3MNC4PD6FNTTMQHIFANCNFSM4X4FE4PQ> .

orbeckst

lgtm

package/MDAnalysis/coordinates/chemfiles.py

richardjgowers reviewed Feb 19, 2021

View reviewed changes

Luthaf force-pushed the chemfiles-0.10 branch from d77f887 to 88e88ce Compare February 28, 2021 14:16

Luthaf force-pushed the chemfiles-0.10 branch 2 times, most recently from 6950d1d to 4f2fe62 Compare February 28, 2021 14:23

Luthaf force-pushed the chemfiles-0.10 branch 2 times, most recently from d03e815 to 63007cb Compare March 2, 2021 12:59

Luthaf force-pushed the chemfiles-0.10 branch from 63007cb to 25a26ed Compare March 12, 2021 13:32

Luthaf force-pushed the chemfiles-0.10 branch from 25a26ed to 4158dca Compare March 13, 2021 15:22

Luthaf force-pushed the chemfiles-0.10 branch 6 times, most recently from 1bc8d2d to e2ca585 Compare April 4, 2021 22:57

IAlibay reviewed Apr 4, 2021

View reviewed changes

.github/workflows/gh-ci.yaml Outdated Show resolved Hide resolved

Luthaf force-pushed the chemfiles-0.10 branch from e2ca585 to 08c4e9d Compare April 4, 2021 23:05

IAlibay reviewed Apr 4, 2021

View reviewed changes

.github/workflows/gh-ci.yaml Outdated Show resolved Hide resolved

Luthaf force-pushed the chemfiles-0.10 branch from 2a8fcd2 to dedb21a Compare April 5, 2021 12:08

IAlibay requested changes Apr 5, 2021

View reviewed changes

Luthaf force-pushed the chemfiles-0.10 branch 2 times, most recently from f90b7e6 to 2c94ba0 Compare April 5, 2021 21:49

Luthaf mentioned this pull request Apr 5, 2021

Add CI runner with older versions of chemfiles #3216

Closed

IAlibay added this to the 2.0 milestone Apr 6, 2021

IAlibay requested review from IAlibay and richardjgowers April 6, 2021 18:52

Luthaf force-pushed the chemfiles-0.10 branch from 2c94ba0 to 3a62f46 Compare April 8, 2021 08:54

Luthaf force-pushed the chemfiles-0.10 branch from 3a62f46 to 562bd43 Compare May 2, 2021 11:45

IAlibay approved these changes May 5, 2021

View reviewed changes

testsuite/MDAnalysisTests/coordinates/test_chemfiles.py Show resolved Hide resolved

Luthaf added 5 commits May 7, 2021 08:29

Update to chemfiles 0.10

a57644e

Pin chemfiles to a version >=0.9

359628f

Put conda-forge first in the list of channels

ababc08

Otherwise conda picks outdated version of libssh2 from biobuilds, which links to non-existing libopenssl.so.1.0.0

Add a test for chemfiles version check

d52ae50

Refactor ChemfilesWriter to follow the same code as the XYZ writer

961b996

Luthaf force-pushed the chemfiles-0.10 branch from e12e44c to 961b996 Compare May 7, 2021 06:30

IAlibay reviewed May 7, 2021

View reviewed changes

Update condition checking for missing cell data

2e248b8

orbeckst approved these changes May 8, 2021

View reviewed changes

package/MDAnalysis/coordinates/chemfiles.py Outdated Show resolved Hide resolved

Update package/MDAnalysis/coordinates/chemfiles.py

b836d43

orbeckst assigned richardjgowers and IAlibay May 8, 2021

richardjgowers approved these changes May 8, 2021

View reviewed changes

richardjgowers merged commit a9eafab into MDAnalysis:develop May 8, 2021

Luthaf deleted the chemfiles-0.10 branch May 8, 2021 17:17

fiona-naughton added maintainability Component-Readers labels Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to chemfiles 0.10 #3126

Update to chemfiles 0.10 #3126

Luthaf commented Feb 19, 2021

pep8speaks commented Feb 19, 2021 •

edited

Loading

codecov bot commented Feb 19, 2021 •

edited

Loading

richardjgowers Feb 19, 2021

Luthaf Feb 19, 2021

IAlibay Feb 19, 2021

Luthaf commented Feb 28, 2021 •

edited

Loading

IAlibay commented Feb 28, 2021

Luthaf commented Mar 3, 2021

IAlibay commented Mar 13, 2021

Luthaf commented Mar 13, 2021

Luthaf commented Mar 13, 2021

IAlibay commented Mar 13, 2021

IAlibay commented Mar 13, 2021

Luthaf commented Apr 4, 2021

Luthaf commented Apr 5, 2021

Luthaf commented Apr 5, 2021

IAlibay left a comment

IAlibay left a comment

IAlibay May 7, 2021 •

edited

Loading

Luthaf May 7, 2021

IAlibay May 7, 2021

Luthaf May 7, 2021

Luthaf May 7, 2021

richardjgowers commented May 7, 2021 via email

orbeckst left a comment

Update to chemfiles 0.10 #3126

Update to chemfiles 0.10 #3126

Conversation

Luthaf commented Feb 19, 2021

PR Checklist

pep8speaks commented Feb 19, 2021 • edited Loading

Comment last updated at 2021-05-08 00:15:17 UTC

codecov bot commented Feb 19, 2021 • edited Loading

Codecov Report

richardjgowers Feb 19, 2021

Choose a reason for hiding this comment

Luthaf Feb 19, 2021

Choose a reason for hiding this comment

IAlibay Feb 19, 2021

Choose a reason for hiding this comment

Luthaf commented Feb 28, 2021 • edited Loading

IAlibay commented Feb 28, 2021

Luthaf commented Mar 3, 2021

IAlibay commented Mar 13, 2021

Luthaf commented Mar 13, 2021

Luthaf commented Mar 13, 2021

IAlibay commented Mar 13, 2021

IAlibay commented Mar 13, 2021

Luthaf commented Apr 4, 2021

Luthaf commented Apr 5, 2021

Luthaf commented Apr 5, 2021

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay May 7, 2021 • edited Loading

Choose a reason for hiding this comment

Luthaf May 7, 2021

Choose a reason for hiding this comment

IAlibay May 7, 2021

Choose a reason for hiding this comment

Luthaf May 7, 2021

Choose a reason for hiding this comment

Luthaf May 7, 2021

Choose a reason for hiding this comment

richardjgowers commented May 7, 2021 via email

orbeckst left a comment

Choose a reason for hiding this comment

pep8speaks commented Feb 19, 2021 •

edited

Loading

codecov bot commented Feb 19, 2021 •

edited

Loading

Luthaf commented Feb 28, 2021 •

edited

Loading

IAlibay May 7, 2021 •

edited

Loading