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

fixed and finalized BdG spin configuration #896

Merged
merged 6 commits into from
Mar 1, 2025
Merged

fixed and finalized BdG spin configuration #896

merged 6 commits into from
Mar 1, 2025

Conversation

zerothi
Copy link
Owner

@zerothi zerothi commented Feb 27, 2025

This brought about lots of restructuring because
it was a nightmare to test.

  • tests have added more functionality Now a sisl_allclose returns a dictionary with an appropriate partial(allclose, ...) with suitable defaults for rtol/atol. The keys are numpy data-types

  • Changed the argument for transpose(hermitian=) to transpose(conjugate=). This is much more intuitive.

  • The Cython files got an overhaul. Now the addition of NC/SOC/Nambu spin configuration is off-loaded into a separate routine. This means that if something needs to change down the line, there is only a single place to do so. And then it will work for all the variants, phase, phase_sc and phase3.

    Much simpler, and less error-prone!

    The benchmarks/optimizations/hamiltonian.ipynb was used to assert this did not introduce a performance regression! Same perf as before the commit.

    I still wonder why the dtype=float k=Gamma for <=polarized calculations shows such bad performance, 10 times slower than when using dtype=complex?

  • added more tests to assert that complex-conjugation actually works.

This fixes #873.

@zerothi
Copy link
Owner Author

zerothi commented Feb 27, 2025

@ahkole could you please try this branch? I think (hope it should work).

Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.87%. Comparing base (ad56e5a) to head (7afbefb).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #896      +/-   ##
==========================================
+ Coverage   86.82%   86.87%   +0.04%     
==========================================
  Files         405      405              
  Lines       53308    53369      +61     
==========================================
+ Hits        46286    46365      +79     
+ Misses       7022     7004      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This brought about lots of restructuring because
it was a nightmare to test.

- tests have added more functionality
  Now a `sisl_allclose` returns a dictionary
  with an appropriate `partial(allclose, ...)`
  with suitable defaults for rtol/atol.
  The keys are numpy data-types
- Changed the argument for `transpose(hermitian=)`
  to `transpose(conjugate=)`.
  This is much more intuitive.
- The Cython files got an overhaul.
  Now the addition of NC/SOC/Nambu spin configuration
  is off-loaded into a separate routine.
  This means that if something needs to change down the
  line, there is only a single place to do so. And then
  it will work for all the variants, phase, phase_sc
  and phase3.

  Much simpler, and less error-prone!

  The benchmarks/optimizations/hamiltonian.ipynb
  was used to assert this did not introduce a performance
  regression! Same perf as before the commit.

  I still wonder why the dtype=float k=Gamma
  for <=polarized calculations shows such bad
  performance, 10 times slower than when using dtype=complex?
- added more tests to assert that complex-conjugation actually
  works.
- The transpose function is now tested to its full extend.
  Combinations of conjugate and spin arguments, and doing
  reverse operations are now added as tests to ensure it works.
  This fixed a bug for transpose with overlap matrices of complex
  nature.

This fixes #873.

Signed-off-by: Nick Papior <[email protected]>

fixed remaining transpose problems, added tests

Signed-off-by: Nick Papior <[email protected]>
Signed-off-by: Nick Papior <[email protected]>

re-added file which should not have been deleted

Signed-off-by: Nick Papior <[email protected]>
This is more direct, and also prints the small spin-box values.

Signed-off-by: Nick Papior <[email protected]>
@ahkole
Copy link
Contributor

ahkole commented Feb 28, 2025

Hi, I ran the test I did before in #873 and on first inspection the results are positive!

bands-plot-merged

Here the black line is the SIESTA-BdG bands and the red dots are the bands computed with sisl from the HSX file of the same calculation. I also read the k-points that SIESTA-BdG used to make sure those were the same. (On a sidenote, is there a way with sisl.viz to label the lines and add a legend to the plot? I wasn't able to figure out how to do this).

Which functionality should now be supported with BdG? Then I will try to test most of that.

@zerothi
Copy link
Owner Author

zerothi commented Feb 28, 2025

Hi, I ran the test I did before in #873 and on first inspection the results are positive!

bands-plot-merged

Great, this indeed looks awesome!

Here the black line is the SIESTA-BdG bands and the red dots are the bands computed with sisl from the HSX file of the same calculation. I also read the k-points that SIESTA-BdG used to make sure those were the same. (On a sidenote, is there a way with sisl.viz to label the lines and add a legend to the plot? I wasn't able to figure out how to do this).

Which functionality should now be supported with BdG? Then I will try to test most of that.

Basically everything should be covered now. Pdos, velocity etc. Try it all. ;) and transport things would also be nice to check, I.e. Self energies etc.

Fixed overlap conjugation for transpose calls.

Also added more detailed tests for TRS

Signed-off-by: Nick Papior <[email protected]>
@zerothi
Copy link
Owner Author

zerothi commented Feb 28, 2025

I think the most pressing thing is to do PDOS, velocity, and perhaps some berry-curvature calculations.

I just did the velocity, and it seems to still fulfill the mirror symmetry around the states. Which kind of makes sense since the BS is symmetric.

Well, perhaps I should just merge this, it is tedious as it stands to have this development waiting (quite a bit of fixes elsewhere as well).

So if you don't mind, I'll merge this start of next week, then you can just give feedback once it is there.

@ahkole
Copy link
Contributor

ahkole commented Feb 28, 2025

Well, perhaps I should just merge this, it is tedious as it stands to have this development waiting (quite a bit of fixes elsewhere as well).

So if you don't mind, I'll merge this start of next week, then you can just give feedback once it is there.

This sounds fine to me. Some of the features you mention I also wouldn't be sure how to verify since we haven't looked at those with siesta-bdg before, like the Berry curvature. So most I could do for those would be test if doesn't throw errors or give very weird results. PDOS I can compare directly with siesta-bdg so will test that soon.

@zerothi
Copy link
Owner Author

zerothi commented Mar 1, 2025

Agreed, pdos would be the most useful just now!

@zerothi zerothi merged commit cfde869 into main Mar 1, 2025
17 checks passed
@zerothi zerothi deleted the 873-bdg-final branch March 1, 2025 19:40
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.

Support for SIESTA-BdG in sisl
2 participants