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.

Sign up for GitHub

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

Jump to bottom

Remove deprecated hole, and completes py2 removal #2778

Merged
merged 16 commits into from
Jun 22, 2020
Merged

Remove deprecated hole, and completes py2 removal #2778

merged 16 commits into from
Jun 22, 2020

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Jun 20, 2020

Fixes #2541, Towards #2773 and #2739 , Changelog for #2765

Changes made in this Pull Request:

  • Removed original deprecated hole code in favour of new hole2.
  • Removed six, and future from remaining files (note, as Remove deprecated hbonds.hbond_analysis #2746 may take some time to complete, I removed the six & future calls here, even if it will just get deleted in the end).
  • Setup scripts no longer take sub-3.6 python versions.
  • Development status set of 6 - Mature for 2.x.x.

PR Checklist

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

@IAlibay IAlibay requested a review from lilyminium June 20, 2020 13:29
@IAlibay IAlibay changed the title Rm hole Remove deprecated hole, and completes py2 removal Jun 20, 2020
lilyminium
lilyminium approved these changes Jun 20, 2020
View reviewed changes
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.

Thanks @IAlibay!

package/CHANGELOG Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 20, 2020 •
edited
Loading

Codecov Report

Merging #2778 into develop will increase coverage by 0.30%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2778      +/-   ##
===========================================
+ Coverage    91.12%   91.42%   +0.30%     
===========================================
  Files          179      178       -1     
  Lines        23844    23479     -365     
  Branches      3144     3090      -54     
===========================================
- Hits         21727    21465     -262     
+ Misses        1496     1425      -71     
+ Partials       621      589      -32
Impacted Files Coverage Δ
package/MDAnalysis/analysis/__init__.py 100.00% <ø> (ø)
package/MDAnalysis/analysis/hole2/__init__.py 100.00% <ø> (ø)
package/MDAnalysis/analysis/hole2/hole.py 73.19% <ø> (ø)
...ckage/MDAnalysis/analysis/hbonds/hbond_analysis.py 80.22% <100.00%> (-0.17%) ⬇️
analysis/__init__.py 100.00% <0.00%> (ø)

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 2fb1ade...fa48e06. Read the comment docs.

orbeckst
orbeckst requested changes Jun 21, 2020
View reviewed changes
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Looking great, just a minor comment (asv).

benchmarks/asv.conf.json Outdated Show resolved Hide resolved
package/MDAnalysis/analysis/__init__.py Outdated
@@ -59,7 +59,7 @@
:mod:`~MDAnalysis.analysis.helanal`
Analysis of helices with the HELANAL_ algorithm.

:mod:`~MDAnalysis.analysis.hole`
:mod:`~MDAnalysis.analysis.hole2.hole`
Copy link
Member

Choose a reason for hiding this comment

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

Would be nicer to have a page for the hole2 package instead of going to its module. But can be separate PR if it requires work.

Copy link
Member Author

Choose a reason for hiding this comment

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

So it turns out this __init__ text isn't actually used anywhere for docs generation (as far as I can tell), the hole2 package links to: https://www.mdanalysis.org/docs/documentation_pages/analysis/hole2.html#hole-analysis-mdanalysis-analysis-hole2

I can just get rid of the .hole portion if you think it's more appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

I think hole2 is more appropriate.

It looks as if the doc string requires an update anyway... probably getting rid of most of it and just saying something simple for anyone using help(MDA.analysis). But can be different issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it in fa48e06, but on testing on a page for which docs actually do get built for, it seems like it won't link to anything. Either way if the whole thing is going to get re-written, and considering that these docs don't get built anyways, then it might be good enough for now?

package/MDAnalysis/analysis/hole2/__init__.py Show resolved Hide resolved
@IAlibay IAlibay mentioned this pull request Jun 21, 2020
Merged
4 tasks
@richardjgowers
Copy link
Member

@IAlibay sort out the conflicts and we can merge this probably

@richardjgowers richardjgowers merged commit 333bb04 into MDAnalysis:develop Jun 22, 2020
@IAlibay IAlibay mentioned this pull request Jun 23, 2020
Closed
@GeraZerbetto
Copy link

Hello everyone! I am a MDA. Hole user but I am quite interested in the development of this tool. I have been using this module to run analysis on quite long trajectories, resulting in long computing times. I don't know if this is the place of doing such a question (I am very new at Github, so I don't know the manners here): is there a way to make the code run parallel frames using python multiprocessing, so a single trajectory could be run on multiple threads? I am very new to all of this, sorry if this is not the place for this!
Regards!

@IAlibay
Copy link
Member Author

IAlibay commented Jun 25, 2020 •
edited
Loading

Hi @GeraZerbetto,

Welcome to MDA (or at least the github side of things if you are already a use the code 😄 )!

Ideally could you ask this question on either the developer or user discussion email lists?

Roughly, the way we try to organise things are in the following manner:

  1. code usage discussion: user mailing list
  2. code development discussions: developer mailing list
  3. issues & feature requests: github issues
  4. active code contributions: github pull requests

@IAlibay IAlibay deleted the rm-hole branch June 25, 2020 15:36
@GeraZerbetto
Copy link

Hi @IAlibay
Thanks a lot!

@IAlibay IAlibay mentioned this pull request Jun 26, 2020
Closed
5 tasks
PicoCentauri added a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021
@PicoCentauri
Remove deprecated hole, and completes py2 removal (MDAnalysis#2778)
f45a2d4
@fiona-naughton fiona-naughton added maintainability deprecation Deprecated functionality to give advance warning for API changes. labels Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orbeckst orbeckst orbeckst approved these changes

@richardjgowers richardjgowers richardjgowers approved these changes

@lilyminium lilyminium lilyminium approved these changes

Assignees
No one assigned
Labels
deprecation Deprecated functionality to give advance warning for API changes. maintainability
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

remove Python 2 constructs
6 participants
@IAlibay @richardjgowers @GeraZerbetto @orbeckst @lilyminium @fiona-naughton