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

Move all PW basis k-point data to new KpointSet struct #1021

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abussy
Copy link
Contributor

@abussy abussy commented Nov 19, 2024

This PR is a minor refactoring of the PlaneWaveBasis struct. All fields related to K-points in the basis are moved to a new KpointSet struct, in the kpoints.jl file (renamed from Kpoint.jl). As of now, this PR simply moves the K-point code previously located in PlaneWaveBasis.jl to kpoints.jl, with only minor modifications. This allows to:

  • Simplify the abstract structure of a basis: it is now a KpointSet, a FFTGrid, an array of Terms, and a handful of general fields (dvol, architecture, etc.).
  • Moves K-point logic and MPI parallelization concerns outside of the basis
  • Generally improves modularity
  • In the future, will allow the simpler/cleaner creation of other sorts of basis (e.g. the SiriusBasis, which will share most of the same structure)

This is currently a draft, as I would like to discuss the following first:
Throughout the code, there are lots of references to basis.kpoints, basis.kweights, etc. This will not be compatible with the changes proposed here. One could either explicitly go through the KpointSet (basis.kpoint_set.kpoints), or define a functions such as kpoints(basis) or kweights(basis). I am personally for the latter option, as it is more robust in case of further modifications, and also more concise.

@antoine-levitt
Copy link
Member

Hm, I'm not convinced by this. It doesn't feel like we gain much by this abstraction: it's just putting a struct on top on a collection, so increases indirection without much gain in code reuse. I don't think there's something wrong with a "flat" structure (a struct with lots of fields) as opposed to a more hierarchical structure (OOP-style). I really don't like the field(struct) business, as doing that just adds inconsistency. This is future-proofing in the wrong way: future-proofing is done by having clean abstractions that map to clear mathematical concepts, not by adding layers of indirection. What is the actual motivation for this change? Maybe having a clearer idea of possible future extensions would help finding a good design.

Happy to discuss next week. We can try to push more fields into the kpoints. For instance kweights could go as a field in the Kpoint structure. Kcoords_global and Kweights_global were mostly a hack, we can probably get rid of them by recomputing them when needed with a mpi_gather?

The thing I don't like with the current design is the meshing together of basis sets and kpoints, where the two should really be independent. We maybe should have had a basis_rho and a basis_psi[ik], all of which <: AbstractBasis. But then again, there is still a pretty tight coupling between G vectors and kpoints that we use quite a bit, so I'm not so sure. In any case this PR is orthogonal to that issue.

@abussy abussy marked this pull request as draft November 20, 2024 08:30
@abussy
Copy link
Contributor Author

abussy commented Nov 20, 2024

The main reason for increasing the abstraction of the PlaneWaveBasis struct is to simplify (on a high level) what a PW basis contains, so that other types of PW bases can be added in the future. In my case, that would be the SiriusBasis, enabling some offloading to the SIRIUS library.

This way, a new basis would have the same simple structure. I also think that this would simplify multiple dispatch for functions that would act on a generic PW basis. Admittedly, all of this can be achieved by using the current organization, but at the cost of greater code duplication.

I'm happy to talk about this and more next week!

@antoine-levitt
Copy link
Member

Then I would suggest to revisit this when it's clearer what the sirius integration actually needs. I thought also about implementing different bases like finite differences and so on, but it's impossible to design a structure that works for all possible generalizations, we have to pick what we actually need and see what are the right abstractions to make this work without too much code duplication.

@mfherbst
Copy link
Member

mfherbst commented Nov 20, 2024

I agree let's talk next week. Essentially the idea to somehow split this up has been motivated from the Sirius integration. Maybe now is the opportunity to think this through a little more carefully also wrt. what happens with other basis sets.

(Even though we might not fully solve the problem).

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