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

Woods-Saxon confinement #90

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

vanderhe
Copy link
Member

@vanderhe vanderhe commented Jul 4, 2024

Implements the Woods-Saxon confinement potential.

  • generalize sktools to WS confinement and hand over to slateratom
  • switch to numeric confinement evaluation throughout and abstract out
  • implement WS potential in slateratom
  • switch to $W,a,r_0$ representation
  • add example(s)
  • add regression tests
  • validate results

Closes #89.

Please do not squash, but use merge commit.

@vanderhe vanderhe added the enhancement New feature or request label Jul 4, 2024
@vanderhe
Copy link
Member Author

vanderhe commented Jul 4, 2024

@samtsevich I have generalized parts of the infrastructure. To be continued here:

https://github.com/vanderhe/skprogs/blob/d27e6b355a1f526cef853422f9e7a355998bd96c/slateratom/prog/main.f90#L94-L101

@vanderhe
Copy link
Member Author

@samtsevich I have switched over to an all numeric integration of the confinement potential and abstracted out the potentials form. In the future we could now easily add 'arbitrary' potentials without involved modifications. Validation is still pending, but I'm mostly confident that it works as intended, as the results of the previous analytical power compression are reproduced, i.e. the regression tests pass.

@vanderhe vanderhe force-pushed the woodsSaxonConfinement branch 2 times, most recently from 4eb1087 to f292839 Compare July 16, 2024 16:13
@vanderhe
Copy link
Member Author

C_veff.pdf

@vanderhe
Copy link
Member Author

@samtsevich I think this is fully operational now. Any ideas on how to further validate the newly implemented WS compression?

@chiarapan
Copy link

Wow that was fast, thank you so much! Not sure if we can validate it e.g. against published parametrizations (Chou's paper, or my lithium-graphite) because the atomic solvers and probably other finer details are all different and/or not available. Did you already give it a test run with some actual SK table generation?

@vanderhe
Copy link
Member Author

Did you already give it a test run with some actual SK table generation?

I have generated some SK-files but did not perform the actual DFTB calculation. The two-center integration does not explicitly depend on the confinement potential, so I guess it is fine. The new numeric approach to the confinement in the one-center part seems to perfectly reproduce the analytical power compression and the WS implementation follows the same code path (but of course with a different functional form). You have way more experience with the WS compression than I have, would it be possible that you briefly check for a 'qualitatively correct/expected behavior'? But it is not urgent (at least not from my side).

@chiarapan
Copy link

I'll check soon with lithium (for which I haven't yet managed to get a great parametrization with the power compression). I'll update as soon as I test it!

@vanderhe
Copy link
Member Author

I'll check soon with lithium (for which I haven't yet managed to get a great parametrization with the power compression). I'll update as soon as I test it!

Thanks a lot 👍.

Copy link
Member

@aradi aradi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, thanks Tammo! I've made a few comments on the code design.

Comment on lines 126 to 129
type(TPowerConfInp) :: power

!> Woods-Saxon confinement input structure
type(TWsConfInp) :: ws
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the components allocatable and allocate on demand? There is less chance, that they are used uninitialized.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I was already thinking about this, don't know why I ultimately decided to not make use of it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be addressed by the latest commit.

contains

procedure(getPotOnGrid), deferred :: getPotOnGrid
procedure(getSupervec), deferred :: getSupervec
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need getSupervec()? Both implementations are exact identical, if I have seen correctly. Let's introduce it only, if we really need it. Or, alternatively, create a concrete implementation in this base class, so that derived classes have only to define getPotOnGrid()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, I was trying to sharpen my Fortran OOP skills with this PR and would be very interested in your opinion/feedback. The getSupervec() routine is a 1:1 duplicate and does not depend on this. I wasn't quite sure how to handle this case in the most elegant way. Since it does not depend on the compression type, can it be associated with the overarching abstract type (is this what you meant by 'create a concrete implementation in this base class'?) or should it just live on its own in the confinement module?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • An abstract type can have non-deferred (normal) type bound procedures with actual implementation.
  • You could either specify the routine there. You might then use the nopass option, so that this is not even passed to the routine.
  • Alternatively, you just use it as a simple function outside. I don't have any strong opinions on this, but probably the way, how the integrals are calculated is independent from the actual potential form, so probably having it outside as a standalone function is more logical...

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 see 👍. I have seen nopass in other projects before and always wanted to use it, seems like a reasonable opportunity for that. See the latest commit.

@samtsevich
Copy link

@vanderhe
Hey Tammo,
Should we first test this implementation before it is merged into the main repo or is that on your side?
[Nevertheless, we can start playing even with this version, and lets see how that works for our optimization procedure]

@chiarapan
Copy link

I'll check soon with lithium (for which I haven't yet managed to get a great parametrization with the power compression). I'll update as soon as I test it!

Thanks a lot 👍.

Tested, it seems to work fine!

@vanderhe vanderhe marked this pull request as ready for review July 23, 2024 13:15
@vanderhe vanderhe requested a review from aradi July 23, 2024 13:16
@samtsevich
Copy link

@aradi
Can you please review these changes?
On our side, we are ready to start playing with the optimization run

@vanderhe
Copy link
Member Author

@aradi Can you please review these changes? On our side, we are ready to start playing with the optimization run

Bálint currently is still on vacation and will pick up work next week. I would estimate that a realistic time frame for this PR getting reviewed/merged is 1-2 weeks from now. From my side this is the final version, so I would suggest that you resort to this branch until it gets merged into main.

@vanderhe
Copy link
Member Author

vanderhe commented Aug 8, 2024

@aradi Just a request: please do not squash f10a9b4 into any of the other commits when merging, I would like to keep the (fully working) $R_\mathrm{onset}, R_\mathrm{cut}$-representation in the history, if possible.

@vanderhe
Copy link
Member Author

@aradi I had to push an additional commit to this branch that fixes a bug in the shelf search. Nothing dramatic, but the previous version was not recognizing matching calculations in the shelf anymore.

@vanderhe vanderhe added this to the 0.4 milestone Aug 23, 2024
slateratom/lib/confinement.F90 Outdated Show resolved Hide resolved
slateratom/lib/confinement.F90 Outdated Show resolved Hide resolved
test/prog/sktable/LDA-PW91/Non-Relativistic/WS/config Outdated Show resolved Hide resolved
@vanderhe
Copy link
Member Author

@aradi Shall we merge this with a merge commit, as discussed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Woods-Saxon confinement
5 participants