First attempt: panedr and panedrlite #42
Conversation
|
Hello @BFedder! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-07-05 15:58:06 UTC |
Codecov Report
@@ Coverage Diff @@
## master #42 +/- ##
==========================================
- Coverage 82.84% 0.00% -82.85%
==========================================
Files 2 2
Lines 274 279 +5
==========================================
- Hits 227 0 -227
- Misses 47 279 +232
Continue to review full report at Codecov.
|
|
I can't seem to get codecov to be happy with these changes, even though the tests pass... Could someone please help me fix this? |
…Analysis-master to be compatible with PR#41
to match PR41
There was a problem hiding this comment.
The codecov stuff is annoying but I have a suspicion it's actually on the codecov side of things - running pytest locally and generating a coverage file works fine with the expected line coverage. I suspect this is because codecov doesn't understand what's going on in the branch, the first here is probably just to merge and see if codecov fixes itself imho.
.github/workflows/gh-ci.yaml
Outdated
|
|
||
| - 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.
You can revert this back to panedrlite/panedrlite
.github/workflows/gh-ci.yaml
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
python -lc "from panedr import edr_to_df"
So presumably I will need to uncomment line 60 (python -m pip install -v ./panedr), right?
There was a problem hiding this comment.
I have done this now, I guess we will only see on merge if the import test works?
panedrlite/panedrlite/panedr.py
Outdated
| 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.
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.
tests/test_edr.py
Outdated
| @@ -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.
Can you fix these imports to be py3+ only now? The py2 code paths aren't necessary anymore.
|
Now that #33 is merged, should we try merging this PR here as well to see if codecov can fix itself, and if |
|
I have now also added type hints and docstrings to address #30 and #31. |
panedrlite/panedrlite/panedr.py
Outdated
| @@ -403,7 +411,37 @@ def is_frame_magic(data): | |||
| return magic == -7777777 | |||
|
|
|||
|
|
|||
| def read_edr(path, verbose=False): | |||
| all_energies_type = list[list[float]] | |||
There was a problem hiding this comment.
Personal opinion but I prefer type hints to be in full at the call site where possible.
I get that here they are too long and complicated to be readable and am happy with your choice but just where possible.
| return all_energies, all_names, times | ||
|
|
||
|
|
||
|
|
||
| def edr_to_df(path: str, verbose: bool = False): |
There was a problem hiding this comment.
| def edr_to_df(path: str, verbose: bool = False): | |
| def edr_to_df(path: str, verbose: bool = False) -> pd.DataFrame: |
There was a problem hiding this comment.
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.
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.
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.
I took one of the suggestions from there and it seems to work :)
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The code & packaging looks ok to me. I have some minor comments but nothing blocking.
I don't understand why codecov is unhappy, perhaps because of the major renaming. Presumably, this can be fixed once the PR is merged and we have the new repo structure in place.
|
We will have to relax the branch protection to merge—unless someone has an idea how to get coverage to report a high number. |
I've dropped the I think all org admins are admins by default, but we can manually add admins if necessary. |
|
Could the Anyway, I am fine with it. |
I think this is working now - I renamed |
|
Turns out, renaming panedrlite to panedr makes things quite complicated for CI testing... Everything works locally, but the import test fails like this. I have looked around for a while on how to expose different import names, but I think this would be the way to have panedrlite be importable as panedr. My suggested solution here is to go back to having it be |
|
Phew because I was scared of the command line merge 😅. |
PR MDAnalysis#42 ## Work done in this PR * Creates the panedr and panedrlite packages * panedrlite is panedr without pandas * panedr import panedrlite and pandas * update CI accordingly * fix some type hint issues
This is the first attempt at creating an empty panedr package that relies on a "panedrlite" package, so will very likely still need quite a bit of work put into it.
That being said, the way this is organised now allows for panedrlite to be pip-installed. With panedrlite installed, the empy panedr can be installed as well. In the process, pandas is automatically installed. The functions are loaded into the panedr namespace, so can be called as they are now (i.e.
panedr.edr_to_df())I'm hoping that once panedrlite is on PyPI, panedr can be installed without prior installation of panedrlite.