Added compatibility with MDAnalysis 2.4 #36

ianmkenney · 2023-01-27T21:23:48Z

Removed the get_propka function (next version must be 2.0.0)
PropkaTraj constructor accepts atom indices for selections
Pandas Index with a dtype of float64 instead of Float64Index, per deprecation warning from Pandas
Removed removed tests for get_propka and changed the baseanalysis test names to be being the standard
Expanded parameters for test_single_frame_regression
Removed MDAnalysis version constraint in setup.py

- Removed the `get_propka` function (next version must be 2.0.0) - PropkaTraj constructor accepts atom indices for selections - Pandas Index with a dtype of float64 instead of Float64Index, per deprecation warning from Pandas - Removed removed tests for `get_propka` and changed the baseanalysis test names to be being the standard - Expanded parameters for `test_single_frame_regression` - Removed MDAnalysis version constraint in setup.py

ianmkenney · 2023-01-27T22:34:43Z

@orbeckst do you know why this linter might be failing? it does not fail for me locally and I'm using the same version of pylint...

orbeckst · 2023-01-27T23:55:25Z

no idea, sorry

orbeckst

Can we just decide to only support Python ≥ 3.8? If so, the setup.py metadata needs to be up-to-date (and we could remove six etc). We can then also set MDAnalysis>=2.

I am confused why select can be a list/array of indices in addition to a selection str. I can't remember when we added that but in any case, it's unlike any of the other typical MDA analysis functions so given that we do 2.0.0 #37 anyway, i suggest we remove this functionality and make it as simple as possible.

orbeckst · 2023-01-28T00:01:40Z

propkatraj/propkatraj.py

@@ -5,7 +5,7 @@

 from __future__ import print_function, division

-from six import string_types, StringIO, raise_from
+from six import StringIO, raise_from


We could get rid of six and just support Python >=3.8

... but that can be another issue.

If CI is 3.8+ then we should just make that move now.

orbeckst · 2023-01-28T00:03:02Z

propkatraj/propkatraj.py

+            if not isinstance(select, str):
+                self.ag = atomgroup.atoms[select].atoms


Why is this necessary? Can select be a index array??

The doc string says that it can only be a string. If you want it to also be an index array then you need to change the docs and a versionchanged.

However, MDAnalysis analysis functions never use index arrays: either a selection or an AtomGroup. I'd probably just remove the ability to pass anything except a selection string and keep it simple.

Agreed, I'm also opposed to using an index array unless there's a good reason for it.

propkatraj/propkatraj.py

propkatraj/tests/test_regression.py

orbeckst · 2023-01-28T00:11:09Z

propkatraj/tests/test_regression.py

@@ -52,32 +53,8 @@ def pka_from_file(filename):
    return resnums, pkas


-@pytest.mark.parametrize('selection', ['protein', 'array', 'list'])
+@pytest.mark.parametrize('selection', ['protein', 'ag', 'array', 'list'])


I honestly have no idea why we are testing anything except a selection string. As far as I am concerned, I would just test "protein" and perhaps "protein and resid 1-10".

It seems like line 79 was erroneously removed and this one (which was intended for the old method) was kept instead.

@IAlibay I figured that I'd do the union of the two parameter sets. I'll switch it to just protein and ag

propkatraj/tests/test_regression.py

IAlibay

I'll have a look at the pylint failure later today

.github/workflows/gh-ci.yaml

devtools/environment.yml

IAlibay · 2023-01-28T01:50:08Z

propkatraj/propkatraj.py

+            if not isinstance(select, str):
+                self.ag = atomgroup.atoms[select].atoms


Agreed, I'm also opposed to using an index array unless there's a good reason for it.

IAlibay · 2023-01-28T01:52:06Z

propkatraj/tests/test_regression.py

@@ -52,32 +53,8 @@ def pka_from_file(filename):
    return resnums, pkas


-@pytest.mark.parametrize('selection', ['protein', 'array', 'list'])
+@pytest.mark.parametrize('selection', ['protein', 'ag', 'array', 'list'])


It seems like line 79 was erroneously removed and this one (which was intended for the old method) was kept instead.

propkatraj/tests/test_regression.py

…nstall requirement

Remove dependence on six

- instead of original 'protein' use 'default' - added 'protein' to replace 'ag' - New selections can be added move conveniently now, but the use of `PSF_FRAME_ZERO_PKA` gets in the way. Selecting a file from a dictionary based on the selection string would fix this.

ianmkenney · 2023-01-31T21:35:53Z

@IAlibay @orbeckst Linting failed because MDAnalysis was not installed alongside pylint, it had no idea what attributes were defined in the superclass and just assumed I wrote something invalid. This is probably going to be an issue for projects using the cookiecutter.

IAlibay · 2023-01-31T21:39:35Z

@IAlibay @orbeckst Linting failed because MDAnalysis was not installed alongside pylint, it had no idea what attributes were defined in the superclass and just assumed I wrote something invalid. This is probably going to be an issue for projects using the cookiecutter.

That should be easy enough to fix, we should push a self install upstream to the cookiecutter bits.

ianmkenney · 2023-01-31T21:42:39Z

I raised an issue for the cookiecutter.

orbeckst

Looking pretty good, just some issues surrounding MDA and Python versions.

devtools/environment.yml

propkatraj/tests/test_regression.py

setup.py

ianmkenney · 2023-02-01T22:33:24Z

@IAlibay Is the current state of this good for you?

- Only test for selection methods of 'protein'

orbeckst

WIth use of .results (which is good), the docs also need to be changed

doc string
example notebook

propkatraj/propkatraj.py

- document attributes - fix sphinx rst

orbeckst · 2023-02-03T15:53:54Z

propkatraj/propkatraj.py

-        self.failed_times = []
+        self.results.pkas = []
+        self.results.num_failed_frames = 0
+        self.results.failed_frames_log = []


Can we remove failed_frames_log and just use failed_frames?

oops misunderstood:frames vs times, will fix

orbeckst · 2023-02-03T15:54:16Z

propkatraj/propkatraj.py

-                self.num_failed_frames += 1
-                self.failed_frames_log.append(self._ts.frame)
-                self.failed_times.append(self._ts.time)
+                self.results.num_failed_frames += 1


superfluous?

Can we remove failed_frames_log and just use failed_frames?

propkatraj/propkatraj.py

orbeckst · 2023-02-03T16:07:04Z

@ianmkenney sorry, lots of doc updates, please squash eventually or just squash the whole PR

ianmkenney · 2023-02-03T17:37:15Z

@IAlibay do the proposed changes work for you?

IAlibay

Sorry I thought I had pushed the request changes a couple hours ago, but for some reason it's not done it...

IAlibay · 2023-02-03T15:45:09Z

.github/workflows/gh-ci.yaml

+        PYLINTRC: .pylintrc
+      run: |
+        pylint propkatraj
+  pypi_check:


There's some empty line consistency issues all over this yaml 🤣 it's not technically incorrect so I won't block over it, but I don't know how I'm the only one feeling anxious looking at the formatting..

IAlibay · 2023-02-03T15:49:06Z

propkatraj/propkatraj.py

+            perc_failure = (self.results.num_failed_frames / self.n_frames) \
+                           * 100


PEP8 implicit line continuation vs explicit?

Tbh here please do just break PEP8 and put it on one line, line comprehension >>> adhering to 79 chars

I'm typically on board with that, I don't particularly like the column maximum requirement, especially for an indentation based language. I will gladly switch it back.

IAlibay · 2023-02-03T15:51:15Z

setup.py

@@ -25,27 +19,25 @@
      classifiers=['Development Status :: 6 - Mature',
                   'Environment :: Console',
                   'Intended Audience :: Science/Research',
-                   'License :: OSI Approved :: GNU General Public License v2 (GPLv2)',
+                   'License :: OSI Approved :: GNU General Public License v2+ (GPLv2+)',


Suggested change

'License :: OSI Approved :: GNU General Public License v2+ (GPLv2+)',

'License :: OSI Approved :: GNU General Public License v2 or later (GPLv2+)',

See: https://github.com/pypa/trove-classifiers/blob/main/src/trove_classifiers/__init__.py#L276

IAlibay · 2023-02-03T15:54:01Z

setup.py

      packages=['propkatraj'],
      scripts=[],
      license='GPLv3',
      long_description=open('README.md').read(),
      long_description_content_type='text/markdown; variant=GFM',
-      install_requires=['six', 'numpy', 'pandas', 'MDAnalysis<2.0.0', 'propka==3.1']
+      python_requires='>=3.8',
+      install_requires=['numpy', 'pandas', 'MDAnalysis>=2.0.0', 'propka==3.1']


re: propka3.1, I can't fully remember. My suggestion is that after this PR we open a separate one that unpins and see what happens.

If I remember correctly 3.2 is broken, then we fixed things upstream and 3.3 works, see: #20. Probably in the next PR just try to pin directly to 3.3 and see if that runs fine, then we can do >=3.3?

That sounds reasonable.

- PEP8 requirement ignored in favor of clarity - setup.py license: v2+ -> v2 or later

IAlibay

lgtm!

IAlibay · 2023-02-03T20:59:45Z

Are we happy to squash merge?

ianmkenney · 2023-02-03T21:03:46Z

Are we happy to squash merge?

Squash away

ianmkenney added 7 commits January 27, 2023 14:18

Added GitHub actions workflow

c8b0c70

updated dev environment requiremnets

29ed5f0

pytest -n from a plugin?

10f0dc1

Missing pkatraj variable in revised test

378d368

Including pylintrc from MDAKit cookiecutter

6f2eb4e

Python 2 style super call confuses pylint?

ac8aedf

ianmkenney mentioned this pull request Jan 27, 2023

2.0.0 release #37

Closed

8 tasks

orbeckst requested changes Jan 28, 2023

View reviewed changes

IAlibay requested changes Jan 28, 2023

View reviewed changes

ianmkenney added 7 commits January 31, 2023 13:59

Added GitHub CI badge for master

80109b0

Removed find_packages import, added py3.8+ requirement, removed six i…

01b757f

…nstall requirement

Added py311 to tested versions

7d3c2a5

MDA>=2.0.0 needed, remove six

7af70dd

Remove support for selection by index/list

8c1dc3b

Remove dependence on six

Pylint probably failing because MDA not installed

e9def9e

ianmkenney mentioned this pull request Jan 31, 2023

pylint likely to cause tests to fail MDAnalysis/cookiecutter-mdakit#47

Closed

ianmkenney requested review from IAlibay and orbeckst January 31, 2023 21:54

orbeckst requested changes Jan 31, 2023

View reviewed changes

devtools/environment.yml Outdated Show resolved Hide resolved

propkatraj/tests/test_regression.py Outdated Show resolved Hide resolved

setup.py Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

Fix required python and MDA versions

ebcbad9

orbeckst approved these changes Feb 1, 2023

View reviewed changes

Removed __future__ imports

8bb8e13

ianmkenney added 5 commits February 2, 2023 14:12

Formatting for gh-ci.yaml

35c7a61

Moved all results from PropkaTraj to results container

192d62d

Improved selection handling of test_single_frame_regression

1e50bf4

Set license to GPLv2+

6e25161

Simplified test_single_frame_regression

fa47c3e

- Only test for selection methods of 'protein'

ianmkenney requested review from IAlibay and orbeckst February 3, 2023 02:34

orbeckst requested changes Feb 3, 2023

View reviewed changes

propkatraj/propkatraj.py Show resolved Hide resolved

ianmkenney and others added 3 commits February 3, 2023 07:13

Updated documentation to reflect the use of the results container

e53620b

Example notebook should use container

d511a44

documentation updates

963c285

- document attributes - fix sphinx rst

orbeckst reviewed Feb 3, 2023

View reviewed changes

propkatraj/propkatraj.py Show resolved Hide resolved

orbeckst added 3 commits February 3, 2023 08:59

fixed my doc changes

efee7e5

more docs update

626144a

docfix

6bfed36

orbeckst approved these changes Feb 3, 2023

View reviewed changes

IAlibay requested changes Feb 3, 2023

View reviewed changes

Formatting and license fix

3bee6ca

- PEP8 requirement ignored in favor of clarity - setup.py license: v2+ -> v2 or later

ianmkenney requested a review from IAlibay February 3, 2023 20:26

IAlibay approved these changes Feb 3, 2023

View reviewed changes

IAlibay merged commit f3c8f56 into dev Feb 3, 2023

IAlibay deleted the mda2.4 branch February 3, 2023 21:07

This was referenced Feb 3, 2023

Reformat for MDAnalysis 2.0 #34

Closed

Review/update to work with propka >= 3.3.0 #20

Open

		if not isinstance(select, str):
		self.ag = atomgroup.atoms[select].atoms

		perc_failure = (self.results.num_failed_frames / self.n_frames) \
		* 100

	'License :: OSI Approved :: GNU General Public License v2+ (GPLv2+)',
	'License :: OSI Approved :: GNU General Public License v2 or later (GPLv2+)',

Added compatibility with MDAnalysis 2.4 #36

Added compatibility with MDAnalysis 2.4 #36

Conversation

ianmkenney commented Jan 27, 2023

ianmkenney commented Jan 27, 2023 • edited Loading

orbeckst commented Jan 27, 2023

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianmkenney commented Jan 31, 2023

IAlibay commented Jan 31, 2023

ianmkenney commented Jan 31, 2023

orbeckst left a comment

Choose a reason for hiding this comment

ianmkenney commented Feb 1, 2023

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst commented Feb 3, 2023

ianmkenney commented Feb 3, 2023

IAlibay left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay commented Feb 3, 2023

ianmkenney commented Feb 3, 2023

ianmkenney commented Jan 27, 2023 •

edited

Loading

IAlibay left a comment •

edited

Loading

IAlibay left a comment •

edited

Loading