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

First attempt: panedr and panedrlite #42

Merged
merged 26 commits into from
Jul 6, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
f0cfe69
First attempt: panedr and panedrlite
BFedder Jun 26, 2022
839ae56
Attempt to fix CI
BFedder Jun 26, 2022
1d61a8d
reattempt CI
BFedder Jun 26, 2022
a937b42
Attempt: Fix pytest/codecov
BFedder Jun 26, 2022
c02e1f7
Reattempt: Codecov
BFedder Jun 26, 2022
e0bca63
blahMerge branch 'panedrlite' of https://github.com/BFedder/panedr in…
BFedder Jun 26, 2022
a9a9887
fixed git merge file weirdness
BFedder Jun 27, 2022
0af1018
comment out pip install panedr
BFedder Jun 27, 2022
233927c
Update gh-ci.yaml
BFedder Jun 27, 2022
1cbb327
fix pbr versioning in panedrlite
BFedder Jun 27, 2022
10a5727
Merge branch 'master' of https://github.com/MDAnalysis/panedr into MD…
BFedder Jun 27, 2022
7a5806c
Merge branch 'MDAnalysis-master' into panedrlite
BFedder Jun 27, 2022
bf89e03
addressing IAlibay's reviewer comments
BFedder Jun 27, 2022
9632971
merge #33
BFedder Jun 29, 2022
55d5731
panedrlite now mentioned in README
BFedder Jun 29, 2022
85ac62d
fix reST syntax
BFedder Jun 29, 2022
58fe71c
actually fix reST syntax
BFedder Jun 29, 2022
8c14842
Added docstrings and type hints
BFedder Jun 29, 2022
8cc1914
remove pandas type hint for now
BFedder Jun 29, 2022
0f405d9
fixed type hints for older python versions
BFedder Jul 1, 2022
c4c9453
added type hint for edr_to_df
BFedder Jul 3, 2022
f9302be
reverted edr_to_df type hints
BFedder Jul 4, 2022
ff328c3
panedrlite now supports import panedr
BFedder Jul 5, 2022
b61cf65
fixed CI links to panedrlite
BFedder Jul 5, 2022
86d17ef
Fixed CI? changed import in panedr/__init__
BFedder Jul 5, 2022
a04eaf0
reverted to panedrlite
BFedder Jul 5, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/gh-ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,12 @@ jobs:

- name: install package
run: |
python -m pip install -v .
python -m pip install -v ./panedrlite
# python -m pip install -v ./panedr
Copy link
Member

Choose a reason for hiding this comment

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

we should add an extra step to check imports and make sure stuff works.

Something like this would do the trick (not tested the working-directory line so 🤞🏽 )

- name: test imports
  # Exit the git repo in order for pbr to stop auto-picking up version info from the local git data
  working-directory: ../
  run: |
    python -Ic "from panedrlite import edr_to_dict"
    python -lc "from panedr import edr_to_df"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

python -lc "from panedr import edr_to_df"

So presumably I will need to uncomment line 60 (python -m pip install -v ./panedr), right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have done this now, I guess we will only see on merge if the import test works?


- name: run unit tests
run: |
pytest -n 2 -v --cov=panedr --cov-report=xml --color=yes ./tests
pytest -n 2 -v --cov=panedrlite --cov-report=xml --color=yes ./tests
Copy link
Member

Choose a reason for hiding this comment

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

You can revert this back to panedrlite/panedrlite


- name: codecov
uses: codecov/codecov-action@v3
Expand Down
2 changes: 1 addition & 1 deletion panedr/__init__.py → panedr/panedr/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@
__version__ = pbr.version.VersionInfo('panedr').release_string()
del pbr

from .panedr import *
from panedrlite import *
orbeckst marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 3 additions & 0 deletions panedr/requirements.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
panedrlite[pandas]
numpy>=1.19.0
pbr
2 changes: 1 addition & 1 deletion setup.cfg → panedr/setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ author_email = [email protected]
summary = Read and manipulate Gromacs energy files
license = LGPL
description_file =
README.rst
../README.rst
long_description_content_type = text/x-rst
home_page = https://github.com/MDAnalysis/panedr
python_requires = >=3.6
Expand Down
File renamed without changes.
7 changes: 7 additions & 0 deletions panedrlite/panedrlite/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# -*- coding: utf-8 -*-

import pbr.version
__version__ = pbr.version.VersionInfo('panedrlite').release_string()
IAlibay marked this conversation as resolved.
Show resolved Hide resolved
del pbr

from .panedr import *
orbeckst marked this conversation as resolved.
Show resolved Hide resolved
31 changes: 27 additions & 4 deletions panedr/panedr.py → panedrlite/panedrlite/panedr.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@
import sys
import itertools
import time
import pandas
import numpy as np


#Index for the IDs of additional blocks in the energy file.
#Blocks can be added without sacrificing backward and forward
Expand Down Expand Up @@ -75,7 +76,7 @@
Enxnm = collections.namedtuple('Enxnm', 'name unit')
ENX_VERSION = 5

__all__ = ['edr_to_df']
__all__ = ['edr_to_df', 'edr_to_dict', 'read_edr']


class EDRFile(object):
Expand Down Expand Up @@ -395,14 +396,14 @@ def edr_strings(data, file_version, n):

def is_frame_magic(data):
"""Unpacks an int and checks whether it matches the EDR frame magic number

Does not roll the reading position back.
"""
magic = data.unpack_int()
return magic == -7777777


def edr_to_df(path, verbose=False):
def read_edr(path, verbose=False):
Copy link
Member

Choose a reason for hiding this comment

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

Does this PR supersede #33 or is it meant to follow it? (i.e. which should get merged first)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I didn't mean to create this branch here from my branch for #33... Too new to using git still. #33 should probably be merged first, or at least the changes approved. Alternatively, I could replace this version of panedr.py with the one from the master branch (as I originally intended)

begin = time.time()
edr_file = EDRFile(str(path))
all_energies = []
Expand All @@ -427,5 +428,27 @@ def edr_to_df(path, verbose=False):
end='', file=sys.stderr)
print('\n{} frame read in {:.2f} seconds'.format(ifr, end - begin),
file=sys.stderr)

return all_energies, all_names, times


def edr_to_df(path: str, verbose: bool = False):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def edr_to_df(path: str, verbose: bool = False):
def edr_to_df(path: str, verbose: bool = False) -> pd.DataFrame:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a bit complicated because pandas is an optional dependency. If pandas is not installed, the module can't be imported if the type hint is present. I could put the function definition into a try-except statement, but then I would lose the custom ImportError message. What's the best thing to do here?

Copy link
Member

Choose a reason for hiding this comment

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

My 2 cents - sometimes you just have to take the loss. Type hints are just that, over-engineering a solution for an optional import for a method that's ~ 3 lines of code probably isn't worth it.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took one of the suggestions from there and it seems to work :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Spoke to soon - I didn't fully think this through, the solution I tried yesterday won't work I'm afraid. I think we'll have to hold back on annotating the return type of edr_to_df for now, but the function name and doc string are pretty self-explanatory, and mypy wasn't working with that anyway yet

mypy error message: "error: Skipping analyzing "pandas": module is installed, but missing library stubs or py.typed marker
panedrlite/panedrlite/panedr.py:59: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports"

Copy link
Member

Choose a reason for hiding this comment

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

I would say don't worry about it. We lived without typed code for Python 0-3.whatever so I'm sure well be fine without it. Sorry for leading you down the garden path.

try:
import pandas
except ImportError:
raise ImportError("""ERROR --- pandas was not found!
pandas is required to use the `.edr_to_df()`
functionality. Try installing it using pip, e.g.:
python -m pip install pandas""")
all_energies, all_names, times = read_edr(path, verbose=verbose)
df = pandas.DataFrame(all_energies, columns=all_names, index=times)
return df


def edr_to_dict(path: str, verbose: bool = False):
all_energies, all_names, times = read_edr(path, verbose=verbose)
energy_dict = {}
for idx, name in enumerate(all_names):
energy_dict[name] = np.array(
[all_energies[frame][idx] for frame in range(len(times))])
return energy_dict
2 changes: 2 additions & 0 deletions panedrlite/requirements.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
numpy>=1.19.0
pbr
31 changes: 31 additions & 0 deletions panedrlite/setup.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
[metadata]
name = panedrlite
author = Jonathan Barnoud
author_email = [email protected]
summary = Read and manipulate Gromacs energy files
license = LGPL
description_file =
../README.rst
long_description_content_type = text/x-rst
home_page = https://github.com/MDAnalysis/panedr
python_requires = >=3.6
classifier =
Development Status :: 4 - Beta
Intended Audience :: Developers
Topic :: Scientific/Engineering :: Bio-Informatics
Topic :: Scientific/Engineering :: Chemistry
Topic :: Scientific/Engineering :: Physics
License :: OSI Approved :: GNU Lesser General Public License v2 or later (LGPLv2+)
Programming Language :: Python :: 3.6
Programming Language :: Python :: 3.7
Programming Language :: Python :: 3.8
Programming Language :: Python :: 3.9
Programming Language :: Python :: 3.10
Operating System :: OS Independent

[extras]
test =
six
pytest
pandas =
pandas
6 changes: 6 additions & 0 deletions panedrlite/setup.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
from setuptools import setup

setup(
setup_requires=['pbr'],
pbr=True,
)
2 changes: 0 additions & 2 deletions requirements.txt

This file was deleted.

2 changes: 1 addition & 1 deletion tests/test_edr.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import contextlib
import numpy
import pandas
import panedr
import panedrlite as panedr
import re

# On python 2, cStringIO is a faster version of StringIO. It may not be
Copy link
Member

Choose a reason for hiding this comment

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

Can you fix these imports to be py3+ only now? The py2 code paths aren't necessary anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Expand Down