-
Notifications
You must be signed in to change notification settings - Fork 16
Impr/refactor find files #151
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
Conversation
pyerrors/input/openQCD.py
Outdated
@@ -154,7 +184,7 @@ def read_rwms(path, prefix, version='2.0', names=None, **kwargs): | |||
nsrc.append(struct.unpack('i', t)[0]) | |||
if version == '2.0': | |||
if not struct.unpack('i', fp.read(4))[0] == 0: | |||
print('something is wrong!') | |||
print(' something is wrong!') |
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.
Hi Justus, should this line raise an Exception or a Warning? I think the print statement is not very instructive to the user.
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.
Ah, this should probably be an exception. What happens here (to my understanding) is that there should be a 0 in the case of openQCD 2.0. Why this is, I am not sure (maybe @s-kuberski has more insight on this). However I agree, that there should probably be an error message like "Are you sure you are using the right version tag?" or something similar.
78ba8cb
to
f9b3e86
Compare
@@ -7,7 +7,7 @@ | |||
def test_rwms(): | |||
path = './tests//data/openqcd_test/' | |||
prefix = 'sfqcd' | |||
postfix = '.rwms' |
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.
Does this PR change the behavior of the input functions? Would using .rwms
still work?
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.
Originally, it did, however after I realized this would be a bit unwieldy and it would be pretty easy to correct, I corrected it here: fbf1349. Like this, everything works, rwms
, .rwms
, rwms.
(whoever would use this) and .rwms.
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.
Very good, I think the test should then cover a few of these cases.
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.
Sure, why not... however I would suggest to put this into the test_find_files
, as that makes it simpler for later bug-fixing.
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.
Looks good, thanks!
In this PR, I refactored the rad methods from the openQCD module. Simply put, I tried to make a more general standardized function to find the files to read.
I also added some tests to accommodate for the changes, as well as adding a test for the read_ms5_method.
I tried to let most things as they were, but I slightly changed the interface with respect to the "postfix" keyword. The interface should however still work in the old fashion as well