Skip to content

Index-based geometric measurements implementation - #56

Open
ppravatto wants to merge 4 commits into
mainfrom
GeometryTools
Open

Index-based geometric measurements implementation#56
ppravatto wants to merge 4 commits into
mainfrom
GeometryTools

Conversation

@ppravatto

Copy link
Copy Markdown
Collaborator

Implementation of distance, angle, and dihedral calculations based on atom indices, using the cartesian coordinates stored in the MolecularGeometry class.

@ppravatto ppravatto added the enhancement New feature or request label Dec 28, 2025

@lbabetto lbabetto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No particular issues from my side, only 2 points I'd like to raise:

  1. Is there a specific reason why we're "abstracting" these functions in the math.py module? For example, the distance function simply returns the np.linalg.norm of the difference between the two arrays, and then the distance class method in geometry.py just returns the imported function acting on the vectors extracted from the indices (also possibly generating confusion between class method and helper function). Why not simply returning np.linalg.norm(self.coordinates[i] - self.coordinates[j]?
  2. Assuming these functions are purely for internal use and we have a reason to put them in a separate math.py module, should we do some validation on the function inputs to avoid passing something other than 3D arrays if we plan to reuse these functions for future implementations?

@ppravatto

Copy link
Copy Markdown
Collaborator Author

No particular issues from my side, only 2 points I'd like to raise:

  1. Is there a specific reason why we're "abstracting" these functions in the math.py module? For example, the distance function simply returns the np.linalg.norm of the difference between the two arrays, and then the distance class method in geometry.py just returns the imported function acting on the vectors extracted from the indices (also possibly generating confusion between class method and helper function). Why not simply returning np.linalg.norm(self.coordinates[i] - self.coordinates[j]?
  2. Assuming these functions are purely for internal use and we have a reason to put them in a separate math.py module, should we do some validation on the function inputs to avoid passing something other than 3D arrays if we plan to reuse these functions for future implementations?

Good point, thanks for raising this.

The original idea behind introducing math.py was to keep these definitions external to geometry.py. While distance is straightforward (essentially np.linalg.norm(p2 - p1)), angle and dihedral are a bit less trivial, so having dedicated functions felt like a clearer and more maintainable reference point.

Also, keeping them in a separate module would make them easier to reuse in the future, since they conceptually operate on coordinates rather than being strictly tied to the MolecularGeometry class.

That said, I agree there is currently some ambiguity: geometry.distance works with indices, while math.distance works with coordinates, which could be confusing.

I see two possible options:

  1. Move everything into geometry.py and remove math.py for simplicity (and re-extract later if needed).
  2. Keep math.py, but make the distinction clearer by renaming the functions (e.g. compute_3d_distance, compute_3d_angle, etc.). This would also justify adding input validation (e.g. enforcing 3D vectors) and make their scope more explicit.

Happy to go with whichever direction you think fits better. My slight preference would be option 2 for clarity and future reuse, but no strong opinion.

@lbabetto

Copy link
Copy Markdown
Contributor

My personal preference is simplicity, so until we need to implement something complex enough to warrant a standalone module, I'd be for keeping everything in geometry.py for the time being.

Even if angle and dihedral calculations may not be immediate for everyone, IMO a brief comment explaining that we're essentially just doing a couple of dot products should be enough to get most present and future developers up to speed :)

Guess we need a tie-breaker @AresValley!

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.

2 participants