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

Use absolute file paths in ITPParser #3108

Merged
merged 22 commits into from
Apr 23, 2021
Merged

Use absolute file paths in ITPParser #3108

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
7f3af66
abspath call to find_path
aditya-kamath Jan 24, 2021
eb6b8aa
Update CHANGELOG
aditya-kamath Jan 24, 2021
45fa573
Update changelog
aditya-kamath Jan 24, 2021
26806fc
Slight change to ITPParser
aditya-kamath Jan 25, 2021
ba1e31d
Merge branch 'develop' into ITPParserfix
aditya-kamath Mar 10, 2021
9c9ed50
test for itpparsefix
aditya-kamath Mar 11, 2021
f6228a4
Merge branch 'develop' into ITPParserfix
aditya-kamath Mar 11, 2021
62307b6
fix PEP8 issues
aditya-kamath Mar 11, 2021
0708e65
Merge branch 'ITPParserfix' of https://github.com/aditya-kamath/mdana…
aditya-kamath Mar 11, 2021
a69ecff
PEP8 fix
aditya-kamath Mar 11, 2021
c7b2fb0
Create temporary directory when testing
aditya-kamath Mar 15, 2021
16cad3d
Fix PEP8 errors
aditya-kamath Mar 22, 2021
dda1efa
Fixed tests to demonstrate relative path and relative string
aditya-kamath Mar 30, 2021
3f12b43
Merge branch 'develop' into ITPParserfix
aditya-kamath Mar 30, 2021
13273cb
Try to #include itp file
aditya-kamath Apr 2, 2021
4d7fe44
Update test_itp.py
aditya-kamath Apr 7, 2021
54f90d1
Merge branch 'develop' into ITPParserfix
aditya-kamath Apr 7, 2021
abc9bb3
Adding another test!
aditya-kamath Apr 10, 2021
34222e5
Merge branch 'develop' into ITPParserfix
aditya-kamath Apr 10, 2021
3a1b2b5
Correct PEP8 errors
aditya-kamath Apr 15, 2021
b14e1e5
minor pep8
aditya-kamath Apr 15, 2021
84d0fd4
minor pep8 2
aditya-kamath Apr 15, 2021
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
1 change: 1 addition & 0 deletions package/CHANGELOG
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ The rules for this file:
* 2.0.0

Fixes
* ITPParser now accepts relative paths (Issue #3037)
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
* ITPParser now accepts relative paths (Issue #3037)
* ITPParser now accepts relative paths (Issue #3037, PR #3108)

* ValueError raised when empty atomgroup is given to DensityAnalysis
without a user defined grid. UserWarning displayed when user defined
grid is provided. (Issue #3055)
Expand Down
3 changes: 1 addition & 2 deletions package/MDAnalysis/topology/ITPParser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,10 +265,9 @@ def find_path(self, path):
current_file = self.current_file

try:
path = path.name
path = os.path.abspath(path.name)
Copy link
Member

Choose a reason for hiding this comment

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

This try/except is to check whether path is a string (which triggers the AttributeError) or a Path object from pathlib (so Path.path is the string version). Does your fix still work if a relative string path is passed in instead, triggering the AttributeError?

By the way, your test should check for both, just in case :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @lilyminium, that's a good observation! However, if the user isn't providing a string and triggering this AttributeError, it's probably doing the right thing by just passing and eventually raising an IO error. I can write a test to check if an IOerror is raised when a relative string is passed but not as a string.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @aditya-kamath,

if the user isn't providing a string and triggering this AttributeError

If a string is provided and the AttributeError is triggered, does your fix still work? The IOError test is probably not necessary. However, your test should check that your fix applies to both a relative Path (https://docs.python.org/3/library/pathlib.html) and a relative string.

(so Path.path is the string version)

This was a typo, sorry. Path.name returns the base name of the Path, as a string.

Copy link
Contributor Author

@aditya-kamath aditya-kamath Mar 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, the answer to if it will work when there's an Attribute error is no! :(
I'm assuming that the situation when this will happen is when os.path.abspath is not able to convert a relative string to an absolute path. I'll write the two situations in my test to see if there's a case when this might happen! :D

except AttributeError:
pass

current_dir = os.path.dirname(current_file)
dir_path = os.path.join(current_dir, path)
if os.path.exists(dir_path):
Expand Down
11 changes: 11 additions & 0 deletions testsuite/MDAnalysisTests/topology/test_itp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,3 +335,14 @@ def test_missing_endif(self):
with pytest.raises(IOError):
with self.parser(ITP_no_endif) as p:
top = p.parse(include_dir=GMX_DIR)

aditya-kamath marked this conversation as resolved.
Show resolved Hide resolved
class Testrelativepath:
Copy link
Member

Choose a reason for hiding this comment

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

Please follow Python convention and use camel casing here if you create a class. However, since you only have one tests, I would suggest that you don't need a class for this test. Instead, you could structure test_relpath as a standalone function.


def test_relpath(self, tmp_path):
content="[ atoms ]\n" +\
Copy link
Member

Choose a reason for hiding this comment

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

It would be neater here to use """ multiline strings. I don't think the indentation will matter to the ITPParser.

" 1 H 1 SOL HW1 1 0.41 1.00800"
d = tmp_path / "sub"
d.mkdir()
p = d / "test.itp"
p.write_text(content)
u = mda.Universe(tmp_path / "sub/test.itp", format='ITP')
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 this would still be an absolute path. Instead, I would suggest using the tmpdir fixture, creating a new directory inside that temporary one, and making sure that the relative path you test looks something like "../test.itp".

Copy link
Contributor Author

@aditya-kamath aditya-kamath Mar 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @lilyminium, I've been looking at pytest tools and os.paths. I see that there's a command called relative_to in the pathlib module which computes a relative path to a specified directory. My approach would have been to make a tmpdir and set that as 'observer' directory and use that command (relative_to) to find the path that it sees.

As it turns out, the relative_to command only works when computing paths from parent directory reference, not sibling directory. So, I can't compute something like '../dir'. The second thing is even if I manually create a relative string which tmpdir would use to read my temporary itp file, I am not sure how to ask pytest to run from my tmpdir while performing this test.

I might have missed something here, especially because I am new to pytest, but I am happy to hear any advice you have here!

Copy link
Member

@lilyminium lilyminium Mar 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @aditya-kamath,

If you go into your temporary directory, write a file called "test.itp", make a new subdirectory, and change into that directory, you should be able to test that file with the path "../test.itp". You might like the tmpdir.as_cwd context manager for that.

Edit: see this StackOverflow question for examples and notes https://stackoverflow.com/questions/55658866/how-to-get-the-temporary-path-using-pytest-tmpdir-as-cwd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great advice! I figured the tests out. Just a note that I've done the reverse of what you might expect. Instead of testing a relative path and then converting it to a relative string. I've first tested a relative string and then followed it up by converting it to a path. Hope that's alright!