Skip to content

Conversation

@matteobecchi
Copy link
Collaborator

@matteobecchi matteobecchi commented Dec 1, 2025

Requested Reviewers: @andrewtarzia @SimoneMartino98

  • The 2-nn intrinsic dimension estimator is some old code that I found and thought could be useful.
  • At the moment, I implemented the "array -> xyz file" function in utilities because to pass through a Trj object you need to add a lot of stuff (atoms topology, tilmestep) that could be unnecessary for someone simply wanting to save some coordinates and open it with Ovito (es, for a toy model or CG simulation).

@matteobecchi matteobecchi self-assigned this Dec 1, 2025
@matteobecchi matteobecchi added the enhancement New feature or request label Dec 2, 2025
@matteobecchi
Copy link
Collaborator Author

The plan now is:

  • We add this simple function that does array -> xyz without passing from a Trj, to keep things simple
  • We also add a Trj method that saves an extended xyz with the trajectory and a series of Insights passed by the user.

@matteobecchi matteobecchi marked this pull request as ready for review December 2, 2025 10:41
Copy link
Collaborator

@SimoneMartino98 SimoneMartino98 left a comment

Choose a reason for hiding this comment

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

i think we need some pytests more for this ? + a comment

overall great job, this is super useful.

return pd.DataFrame(data)


def save_xyz_from_ndarray(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe make a point more on the fact that we mean position coordinate (that must be (frame, atoms ,3)?
Or extend the function to create a general XYZ with fewer or more than 3 columns? The user in that case must specify the identity of each column (like Ovito).

Copy link
Collaborator

Choose a reason for hiding this comment

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

just a thought: In this case using pandas could help

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I specified better what this function is for, and added a note suggesting to use Trj if the system has more complexity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Surely pandas give more control but I don't want to force the user to use pandas... for instance I have no idea how to use it. Is this bad? -.-"

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not so difficult to use, and it contains some useful utilities for organizing datasets (remove cols, add cols, add rows). But anyway, it was just a possibility, keep this one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not bring pandas into this, and if we do, we will use polars (pandas in rust).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's always another, better version of everything...

Copy link
Collaborator

@SimoneMartino98 SimoneMartino98 left a comment

Choose a reason for hiding this comment

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

everything ok for me now; good job 👍



@numba.jit # type: ignore[misc]
@numba.jit # type: ignore[untyped-decorator]
Copy link
Collaborator

Choose a reason for hiding this comment

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

njit == numba.jit?

Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure but:
@njit is short for @jit(nopython=True).

so, to be sure, i would add the argument.

but let's check it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I'll make it consistent with the lens.py file.

@matteobecchi matteobecchi merged commit 5f2d928 into GMPavanLab:main Dec 4, 2025
11 checks passed
@matteobecchi matteobecchi deleted the devel branch December 4, 2025 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants