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.

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

Deprecate write-only file modes. #238

Merged
merged 6 commits into from
May 8, 2023
Merged

Conversation

joaander
Copy link
Member

@joaander joaander commented Apr 27, 2023

Description

Add file modes that mimic those in h5py.

Motivation and Context

GSD files must read the header and indices in order to write to the file, and files must be open in binary mode. Adopting the simplified set of file modes from h5py will make it easier for users to choose the correct mode. Work on #236 prompted this change, as the existence of the GSD_APPEND mode resulted in several bugs during development.

How Has This Been Tested?

Existing unit tests pass and provide the expected FutureWarning messages.

Change log

Added:

* File modes ``'r'``, ``'r+'``, ``'w'``, ``'x'``, and ``'a'``.
  (`#238 <https://github.com/glotzerlab/gsd/pull/238>`__).

Deprecated:

* File modes ``'wb'``, ``'wb+'``, ``'rb'``,  ``'rb+'``, ``'ab'``, ``'xb'``, and ``'xb+'``.  
  (`#238 <https://github.com/glotzerlab/gsd/pull/238>`__).
* [C API] ``GSD_APPEND`` file open mode.
  (`#238 <https://github.com/glotzerlab/gsd/pull/238>`__).

Checklist:

@joaander joaander requested review from a team and tommy-waltmann and removed request for a team April 27, 2023 19:08
Copy link

@tommy-waltmann tommy-waltmann left a comment

Choose a reason for hiding this comment

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

According to the documentation in gsd/hoomd.py there never were any write only file modes. The documentation for wb, xb, and ab all contain the phrase "reading and writing". In fact, the wb/wb+ (xb/xb+) modes have the same exact description.

@joaander
Copy link
Member Author

joaander commented May 1, 2023

Following the naming scheme in Python's open function, the modes without the '+' should be write only. In gsd, they are not - thus they should be removed to avoid confusion.

In GSD 1.x, these modes were effectively write only, even though they needed to open the file in read/write mode to accomplish this. I failed to remove the modes without '+' in the 2.x release - this deprecation will fix that in gsd v3.

@tommy-waltmann
Copy link

Thinking forward to version 3, should we support write mode strings where the characters w/x/a, b, and + can appear in any order like the python open function supports?

@joaander
Copy link
Member Author

joaander commented May 2, 2023

Thinking forward to version 3, should we support write mode strings where the characters w/x/a, b, and + can appear in any order like the python open function supports?

We could. Would you like to make the change on this branch?

@joaander
Copy link
Member Author

joaander commented May 2, 2023

Alternately, we could follow the pattern of h5py: https://docs.h5py.org/en/stable/high/file.html#file-open
Interestingly, h5py makes the 'a' mode read/write an existing file or create if needed. This is HOOMD-blue's behavior as well, but gsd 2.x provides no equivalent mode. This is an oversight.

@tommy-waltmann if I adopt the h5py scheme, would you still want to allow the characters in any order: e.g. 'r+' gives the same results as '+r'?

@tommy-waltmann
Copy link

@tommy-waltmann if I adopt the h5py scheme, would you still want to allow the characters in any order: e.g. 'r+' gives the same results as '+r'?

h5py doesn't support the + coming before the r, so I would enforce r+ and error on +r.

@tommy-waltmann
Copy link

I also like adopting the h5py convention that the b be left implicit.

@joaander joaander requested review from a team and SchoeniPhlippsn and removed request for a team May 3, 2023 12:13
Copy link

@tommy-waltmann tommy-waltmann left a comment

Choose a reason for hiding this comment

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

Just a couple of small comments.

gsd/__main__.py Outdated Show resolved Hide resolved
gsd/fl.pyx Outdated Show resolved Hide resolved
gsd/hoomd.py Outdated Show resolved Hide resolved
@joaander joaander merged commit 4914bb2 into trunk-minor May 8, 2023
@joaander joaander deleted the deprecate-write-only-modes branch May 8, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants