-
Notifications
You must be signed in to change notification settings - Fork 89
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
Update constructor for positions and lattice in Model and basis #675
base: master
Are you sure you want to change the base?
Conversation
|
||
Creates a new basis, identical to `basis`, but different `lattice` or `positions`. | ||
""" | ||
function PlaneWaveBasis(basis::PlaneWaveBasis, lattice::AbstractMatrix, |
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.
How about just PlaneWaveBasis(basis, model)?
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.
Hmm. Not so sure. My idea was to have a way to update position and lattice really from a high level with one function and exactly not needing to go through the separate construction of model and basis "by hand".
basis.Ecut, basis.fft_size, basis.variational, | ||
basis.kcoords_global, basis.kweights_global, | ||
basis.kgrid, basis.kshift, basis.symmetries_respect_rgrid, | ||
basis.comm_kpts) |
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.
Hm this can have unintended consequences, ie the model can change symmetries, lattice leading to different fft_size, etc...
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.
There are two possible intended behaviors here: either keep the basis as much the same as possible, or keep it as close to a new basis as possible. This sort of does neither. Not sure what to do here.
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 it should be as close to the new basis as possible, i.e. it should be a new consistent basis with the new structure. So yes, I think this should change the FFT size if needed. I did not think about that.
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.
Then I'm not sure this can be done sensibly, because we have different ways of constructing a pwbasis. I think we should move towards encapsulating the kpoint grid (and possibly the G grid ?) better,and then have a unique pwbasis(::Model, ::Kgrid) constructor, which would then make what you want to do here more straightforward.
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.
By Kgrid I mean the specification (uniform/custom/symmetrized/shifted/..., with symmetries or not, not the actual instantiation) of the kgrid. Same for the G grid (fft_size, variational, )
No description provided.