-
Notifications
You must be signed in to change notification settings - Fork 628
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
Support Pathlike objects in fileio #1813
Conversation
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 made a few suggestions, but looks good.
qutip/fileio.py
Outdated
if file.suffix != '.qu': | ||
file.with_suffix('.qu') |
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.
Previously the code always added .qu
regardless -- e.g. foo.qu
would be saved as foo.qu.qu
and foo.txt
would be saved as foo.txt.qu
. The new code saves foo.qu
as foo.qu
and foo.txt
as foo.qu
.
I'm not a fan of the old behaviour particularly, but the new behaviour of replacing the suffix isn't much better -- both are unnecessary complications that just make life harder for QuTiP users. Typing qsave("foo.qu")
is more explicit and is less for the user to remember about the underlying function works.
I would suggest we keep the old behaviour for 4.7 (i.e. in this PR) -- e.g. file = file.with_suffix(file.suffix + ".qu")
.
If you are interested, you could then open up a follow up PR for QuTiP 5 (the dev.major
branch) and remove the suffix addition entirely there.
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 brought back the functionality and will open a pull request for v5.
qutip/tests/test_fileio.py
Outdated
ops_in = [qutip.sigmax(), | ||
qutip.num(_dimension), | ||
qutip.coherent_dm(_dimension, 1j)] | ||
filename = _random_file_name() | ||
if include_suffix: |
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.
If we remove the if statement that checks for .qu
as I suggested above, then we can also delete the extra test case here. Although perhaps testing a few suffixes is not a bad idea. E.g. change 'include_suffix', [True, False]
to 'suffix', ['', '.qu', '.dat']
.
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.
Testing for different suffixes is a good idea, the code can then be reused later in v5.
1124e1f
to
3954381
Compare
@labay11 Thank you! I've approved the changes and marked them for inclusion in 4.7. I'll merge once the test runs have succeeded. |
pickle.dump(data, fileObject) | ||
fileObject.close() | ||
file = Path(name) | ||
file = file.with_suffix(file.suffix + ".qu") |
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.
file isn't used in the pickle or when opening the fileObject.
if file.suffix != '.qu': | ||
file.with_suffix('.qu') | ||
|
||
with open(name, "rb") as fileObject: |
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.
The behaviour here has changed as it no longer automatically includes the .qu. I believe this was meant to be dealt with by the file.with_suffix above, but the file Path object is never used.
Description
Related issues or PRs
fix #1184
Changelog
qsave
andqload
now supportpathlib.Path
objects.