-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add XYZ file support #13
base: master
Are you sure you want to change the base?
Conversation
able to read elements now
I'm unsure whether what I did with references and the GIL to get the rust side to compile is sound and idomatic but it does pass my simple tests.
Hi Josh -- thanks for this!
|
benchmarks/benchmark_xyz.py
Outdated
benchmark: typing.Any, | ||
tmp_path: pathlib.Path, | ||
) -> None: | ||
xyz_structures = create_xyz_structures() |
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 would create a fixture
@fixture
def structures() -> list[flour.XyzData]:
num_atoms = 100
rng = np.random.default_rng(11)
return [
flour.XyzData(
comment="Test comment",
elements=["C"]*num_atoms,
positions=rng.random((num_atoms, 3)) * 100,
)
for _ in range(500)
]
which i would use in the benchmarks
def benchmark_write_xyz(
benchmark: typing.Any,
tmp_path: pathlib.Path,
structures: list[flour.XyzData],
) -> None:
...
tests/test_xyz.py
Outdated
assert xyz_data_1.comment == xyz_structures[1].comment | ||
assert np.all(np.equal(xyz_data_1.elements, xyz_structures[1].elements)) | ||
assert np.all(np.isclose(xyz_data_1.positions, xyz_structures[1].positions)) | ||
|
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.
needs a newline
|
||
|
||
class XyzData: | ||
elements: list[str] |
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.
use a numpy array of uint8 called atoms
this will be more consistent with cube file interface
#[pyclass] | ||
struct XyzData { | ||
#[pyo3(get)] | ||
elements: Vec<String>, |
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.
use
#[pyo3(get)]
atoms: Py<PyArray1<u8>>,
instead of elements
this is more consistent with the cube file interface
also using Vec<String>
would be less performant because of redundant copies and could lead to surprising behvaiour since it creates copies each time it is accessed -- see https://pyo3.rs/v0.20.0/faq.html#pyo3get-clones-my-field
#[pyo3(get)] | ||
elements: Vec<String>, | ||
#[pyo3(get)] | ||
comment: String, |
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.
Use Py<PyString>
https://pyo3.rs/main/doc/pyo3/types/struct.pystring
here is an example of how to use the type
use pyo3::prelude::*;
use pyo3::types::PyString;
#[pyclass]
struct Foo {
#[pyo3(get)]
x: Py<PyString>,
}
#[pymethods]
impl Foo {
#[new]
fn new(py: Python, x: &str) -> PyResult<Self> {
Ok(Foo {
x: PyString::new(py, x).into_py(py),
})
}
}
src/lib.rs
Outdated
let mut multi_xyz_data: Vec<XyzData> = vec![]; | ||
let contents = fs::read_to_string(path)?; | ||
let mut lines = contents.lines(); | ||
loop { |
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 would re-write this using awhile-let
loop
while let (Some(first_line), Some(second_line)) = (lines.next(), lines.next()) {
...
}
Here are notes and questions that have come up so far. Most notably, going from 1 to 2 supported file formats means that a number of decisions came up regarding how the code and functionality associated with different file formats would be related.
Remaining todo list:
closes joshkamm#1