-
Notifications
You must be signed in to change notification settings - Fork 7
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
Changes from 12 commits
f0cfe69
839ae56
1d61a8d
a937b42
c02e1f7
e0bca63
a9a9887
0af1018
233927c
1cbb327
10a5727
7a5806c
bf89e03
9632971
55d5731
85ac62d
58fe71c
8c14842
8cc1914
0f405d9
c4c9453
f9302be
ff328c3
b61cf65
86d17ef
a04eaf0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
- 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
panedrlite[pandas] | ||
numpy>=1.19.0 | ||
pbr |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
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
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
|
@@ -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): | ||||||
|
@@ -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): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
begin = time.time() | ||||||
edr_file = EDRFile(str(path)) | ||||||
all_energies = [] | ||||||
|
@@ -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): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that being said, one of these solutions might work: https://stackoverflow.com/questions/61384752/how-to-type-hint-with-an-optional-import There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 mypy error message: "error: Skipping analyzing "pandas": module is installed, but missing library stubs or py.typed marker There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
numpy>=1.19.0 | ||
pbr |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from setuptools import setup | ||
|
||
setup( | ||
setup_requires=['pbr'], | ||
pbr=True, | ||
) |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
There was a problem hiding this comment.
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 🤞🏽 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So presumably I will need to uncomment line 60 (
python -m pip install -v ./panedr
), right?There was a problem hiding this comment.
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?