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

Introduce basis_size #79

Merged
merged 2 commits into from
Aug 29, 2024
Merged

Introduce basis_size #79

merged 2 commits into from
Aug 29, 2024

Conversation

cortner
Copy link
Member

@cortner cortner commented Aug 27, 2024

This PR replaces length(basis) with length_basis(model_or_basis) and implements a default fallback to length. Because of the fall-back this should be fully backward compatible.

This allows having a model have a basis and throughout the assembly one passes around the model rather than the model.basis. It cleans up a few things in ACEpotentials.

@cortner cortner requested a review from wcwitt August 27, 2024 21:19
@wcwitt
Copy link
Collaborator

wcwitt commented Aug 28, 2024

I think basis_size might be a slightly clearer name for this concept, but length_basis is also fine.

@cortner
Copy link
Member Author

cortner commented Aug 28, 2024

I'm happy to change the name if you prefer. For context: in ACEpotentials I've now introduced several basis versions of functions such as

energy_basis
forces_basis
virial_basis
length_basis

The attempt here is to be consistent. But it doesn't have to be consistent. In light of the above, if you want me to change that name, let me know and I'll go ahead.

@wcwitt
Copy link
Collaborator

wcwitt commented Aug 28, 2024

Since the first three return a basis (I assume), but the latter returns a length, I'm not too worried about consistency. From there, basis_size and/or basis_length sound more natural than length_basis.

I would also be tempted to do nothing here and instead require length(model) = length(model.basis).

But any of these are fine with me - you choose.

@cortner
Copy link
Member Author

cortner commented Aug 28, 2024

I'm nervous about length(model) because I can envision that the model length can mean a number of different things.

I'll use basis_size for now. Might be good to not clash with names in other modules - avoid confusion.

Is it ok if I make that change and then merge?

@wcwitt
Copy link
Collaborator

wcwitt commented Aug 28, 2024

Sounds good, and yes.

@cortner cortner changed the title Introduce length_basis Introduce basis_size Aug 29, 2024
@cortner cortner merged commit c94ce7c into main Aug 29, 2024
9 checks passed
@cortner cortner deleted the co/lenbasis branch August 29, 2024 03:24
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.

2 participants