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

Update constructor for positions and lattice in Model and basis #675

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mfherbst
Copy link
Member

No description provided.


Creates a new basis, identical to `basis`, but different `lattice` or `positions`.
"""
function PlaneWaveBasis(basis::PlaneWaveBasis, lattice::AbstractMatrix,
Copy link
Member

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)?

Copy link
Member Author

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".

src/Model.jl Show resolved Hide resolved
basis.Ecut, basis.fft_size, basis.variational,
basis.kcoords_global, basis.kweights_global,
basis.kgrid, basis.kshift, basis.symmetries_respect_rgrid,
basis.comm_kpts)
Copy link
Member

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...

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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, )

@mfherbst mfherbst mentioned this pull request Feb 8, 2023
@epolack epolack mentioned this pull request Mar 2, 2023
2 tasks
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