-
Notifications
You must be signed in to change notification settings - Fork 13
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 option to fill all species when computing PowerSpectrum
#214
Conversation
Here is a pre-built version of the code in this pull request: wheels.zip, you can install it locally by unzipping |
I think the ideal implementation would be something that has an option |
keys_to_move = Labels( | ||
names="species_neighbor", | ||
values=np.unique( | ||
spherical_expansion_1.keys["species_neighbor"] | ||
).reshape(-1, 1), | ||
) |
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 think I'd be fine with this being the only behavior: we are not introducing too much zeros since we are moving the two spherical expansions separately, so we could just always do this and not have an extra option.
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.
Okay, even better if we do not overload the parameters with more and more options.
That could work as well, but I don't love that we are introducing a new set of parameters just for this. This feels very similar to the job done by
The issue with making it part of the compute method is that we now have different set of options for Rust calculators and this Python calculator, which I don't love. I agree it feels much more natural to have it here though! |
Possible fix for #213.
This adds the option
fill_species_neighbor
to the compute method ofPowerSpectrum
. Setting this toTrue
changeskeys_to_move
when performingkeys_to_properties
in such a way that it contains all species of the provided systems.I am not super happy with the name of the parameter but I think the it should be part of the the compute method.
📚 Documentation preview 📚: https://rascaline--214.org.readthedocs.build/en/214/