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

Change return type of frame cell to Option #42

Open
seatonullberg opened this issue May 3, 2023 · 3 comments
Open

Change return type of frame cell to Option #42

seatonullberg opened this issue May 3, 2023 · 3 comments

Comments

@seatonullberg
Copy link

Many file formats do not always store a bounding box. I think it would be a significant usability improvement if getting the cell from a frame behaved the same way as getting the velocities (#25).

Currently, if the frame does not contain a cell it will silently return a default cell that is initialized with all matrix elements set to zero. In order to check for this one must iterate over the matrix elements and assume that a matrix full of zeros means the cell was not found in the file.

I would be happy to make a pull request that addresses this from the Rust side if it is something that doesn't need to be handled in the underlying C++ code.

@Luthaf
Copy link
Member

Luthaf commented May 4, 2023

The way this is handled in chemfiles is with the CellShape enum, where an Infinite cell correspond to non-periodic systems. Would checking this first work for you?

We could also change the return type of UnitCell::matrix() to be an option, returning None when the cell is infinite.

@seatonullberg
Copy link
Author

Thanks for the clarification. Checking the CellShape enum will work for my purposes but I also like the idea of returning an option from UnitCell::matrix(). To me that seems like a more rust-y approach than returning a default value.

@Luthaf
Copy link
Member

Luthaf commented May 4, 2023

but I also like the idea of returning an option from UnitCell::matrix().

Since this will be a breaking change, I'd rather do it after the release for 0.10.4.

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

No branches or pull requests

2 participants