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

updated CHEMFILES docs #3128

Merged
merged 4 commits into from
Mar 13, 2021
Merged

updated CHEMFILES docs #3128

merged 4 commits into from
Mar 13, 2021

Conversation

orbeckst
Copy link
Member

Changes made in this Pull Request:

  • add section on using chemfiles for reading and writing
  • fixed broken link to chemfiles formats

PR Checklist

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

- add section on using chemfiles for reading and writing
- fixed broken link to chemfiles formats
@pep8speaks
Copy link

pep8speaks commented Feb 19, 2021

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

Line 263:80: E501 line too long (95 > 79 characters)
Line 264:80: E501 line too long (95 > 79 characters)
Line 265:80: E501 line too long (95 > 79 characters)
Line 266:96: W291 trailing whitespace
Line 276:80: E501 line too long (89 > 79 characters)

Line 23:80: E501 line too long (81 > 79 characters)
Line 26:80: E501 line too long (83 > 79 characters)
Line 26:84: W291 trailing whitespace
Line 27:80: E501 line too long (86 > 79 characters)
Line 28:80: E501 line too long (84 > 79 characters)
Line 28:85: W291 trailing whitespace
Line 32:80: E501 line too long (89 > 79 characters)
Line 33:80: E501 line too long (91 > 79 characters)
Line 35:80: E501 line too long (86 > 79 characters)
Line 40:80: E501 line too long (85 > 79 characters)
Line 40:86: W291 trailing whitespace
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 115:52: W291 trailing whitespace

Comment last updated at 2021-02-22 07:20:56 UTC

@orbeckst
Copy link
Member Author

@Luthaf , perhaps you could have a quick look, too — any comments welcome.

@codecov
Copy link

codecov bot commented Feb 19, 2021

Codecov Report

Merging #3128 (afe5bd4) into develop (f542aa4) will decrease coverage by 0.45%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3128      +/-   ##
===========================================
- Coverage    93.12%   92.67%   -0.46%     
===========================================
  Files          171      171              
  Lines        22706    22706              
  Branches      3209     3209              
===========================================
- Hits         21146    21043     -103     
- Misses        1503     1615     +112     
+ Partials        57       48       -9     
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/__init__.py 100.00% <ø> (ø)
package/MDAnalysis/coordinates/chemfiles.py 31.81% <ø> (-58.53%) ⬇️

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 f542aa4...afe5bd4. Read the comment docs.

Copy link
Contributor

@Luthaf Luthaf left a comment

Choose a reason for hiding this comment

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

This looks good to me!

package/MDAnalysis/coordinates/__init__.py Show resolved Hide resolved
- link to 0.9.3 chemfiles docs
- document supported versions and helper functions and data
@orbeckst
Copy link
Member Author

@Luthaf do you know why we restricted chemfiles to 0.9 ≤ v < 0.10 in MDAnalysis?

@Luthaf
Copy link
Contributor

Luthaf commented Feb 19, 2021

There were big API changes between 0.8 and 0.9, and 0.10 was far from done when I wrote this code so I did not know if there were going to be more API changes, hence the rather conservative compatibility bounds =). Now I think compatibility can be updated to any 0.9.x or 0.10.x version.

@orbeckst
Copy link
Member Author

orbeckst commented Feb 19, 2021 via email

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @orbeckst!

package/MDAnalysis/coordinates/chemfiles.py Outdated Show resolved Hide resolved
@orbeckst
Copy link
Member Author

orbeckst commented Feb 20, 2021 via email

@lilyminium
Copy link
Member

I suppose I read the italicisation as emphasising that this particular example is for writing a mol2 file only, except that no other examples have been hitherto given. It's a small style thing but I personally wouldn't emphasise that, I suppose. Opinions from everyone else are welcome!

@orbeckst
Copy link
Member Author

If anyone wants to merge it please just "squash and merge" everything into one commit and edit the commit message appropriately.

@IAlibay
Copy link
Member

IAlibay commented Mar 13, 2021

Just re-running CI here (checking for a certain failure we're seeing somewhere else), if it returns green I'll squash-merge.

@IAlibay IAlibay merged commit 6e38aa4 into develop Mar 13, 2021
@IAlibay IAlibay deleted the chemfiles-docs branch March 13, 2021 02:45
@IAlibay IAlibay mentioned this pull request Mar 13, 2021
4 tasks
PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021
## Work done in this PR
- Fixes broken link to the chemfiles format
- Adds chemfiles to list of supported file formats
- Adds examples for reading and writing files using chemfiles
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.

5 participants