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

Extension of transform module #124

Merged
merged 98 commits into from
May 8, 2024
Merged

Extension of transform module #124

merged 98 commits into from
May 8, 2024

Conversation

EnricoTrizio
Copy link
Collaborator

@EnricoTrizio EnricoTrizio commented Apr 8, 2024

Description

Adding some preprocessing tools to the transform module, including descriptors computation and tools.

NB-1 Transform object are supposed not to have trainable parameters.
NB-2 Position-dependent descriptors are implemented only for cubic/orthorhombic cells.

The structure of the module is:

Tools

  • Normalization
  • Switching functions (new)
  • Continuous Histogram (new, derivable histogram using KDE)

Descriptors, acting on positions

  • Multiple descriptors (new, wrapper class to compute multiple descriptors on the same set of positions)
  • Pairwise distances (new, computes ALL pairwise distances from a set of positions)
  • Torsional angle (new, computes the torsional angle from a set of positions + index of 4 involved atoms)
  • Coordination number (new, computes from a set of positions coordination numbers of elements of group A wrt group B)
  • Eigs adjacency matrix (new, computes adjacency matrix eigenvalues. Could be moved to a tutorial)

Already implemented but likely to be kept out of this PR:

  • Radius graph (compute graph inputs in the format: edge sources, edge destinations and edge distances)
  • Radial distribution function (compute RDF from a set of position and optionally the integral of a peak)

Status

  • Ready to go

Copy link
Collaborator

@andrrizzi andrrizzi left a comment

Choose a reason for hiding this comment

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

Looks good! I've left a few comment but it's only nitpicking.

For the structure, it looks ok to me, but I'd tend to have the least possible number of packages/modules called tools or utils. I might be missing the rationale here though. I usually use tools essentially as synonym for utils meaning "anything I don't know where else to put/how else to organize".

Maybe we could simply move all the transform/tools/*.py modules directly in transform/*.py. Some of the elements (like Inverse or Statistics) might deserve their own inverse.py or statistics.py module. This is mostly taste anyway. It might not be the prettiest structure to have a bunch of small modules inside a package but I usually find it helpful when I navigate codebases.

@EnricoTrizio EnricoTrizio marked this pull request as ready for review May 8, 2024 13:43
@EnricoTrizio EnricoTrizio merged commit 14d2711 into main May 8, 2024
12 checks passed
@EnricoTrizio EnricoTrizio deleted the transform branch May 8, 2024 13:51
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.

3 participants