Skip to content

Fixes for native hamiltonian format #888

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

Merged
merged 4 commits into from
Jan 27, 2025
Merged

Conversation

tfrederiksen
Copy link
Contributor

Closes #887

This PR produces the following test1.ham that looks complete and correct:

begin cell
  2.13000000 -1.22975607 0.00000000
  2.13000000 1.22975607 0.00000000
  0.00000000 0.00000000 20.00000000
end cell

supercell 3 3 1

begin atoms
  6 0.00000000 0.00000000 0.00000000
  6 1.42000000 0.00000000 0.00000000
end atoms

begin matrix 0 0 0
 0 1 -2.7
end matrix 0 0 0

begin matrix 1 0 0
 0 0 -0.2
 1 0 -2.7
 1 1 -0.2
end matrix 1 0 0

begin matrix -1 1 0
 0 0 -0.2
 0 1 -0.18
 1 1 -0.2
 1 0 -0.18
end matrix -1 1 0

begin matrix 0 1 0
 0 0 -0.2
 1 0 -2.7
 1 1 -0.2
end matrix 0 1 0

begin matrix 1 1 0
 1 0 -0.18
end matrix 1 1 0

@tfrederiksen
Copy link
Contributor Author

One should perhaps take this opportunity to also include some tests....

@zerothi
Copy link
Owner

zerothi commented Jan 26, 2025

Thanks for this, I can see why this fixes the issue, I will have to amend this to ensure it works for all sc_off cases (in fact, users can specify their own ordering of supercells, so I'll have to check that (shouldn't be difficult!

@tfrederiksen
Copy link
Contributor Author

Thanks for this, I can see why this fixes the issue, I will have to amend this to ensure it works for all sc_off cases (in fact, users can specify their own ordering of supercells, so I'll have to check that (shouldn't be difficult!

Yes, indeed this algorithm assumes a specific ordering.

Copy link

codecov bot commented Jan 26, 2025

Codecov Report

Attention: Patch coverage is 84.61538% with 14 lines in your changes missing coverage. Please review.

Project coverage is 86.82%. Comparing base (39aab90) to head (989830e).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/sisl/io/ham.py 81.57% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #888      +/-   ##
==========================================
- Coverage   86.87%   86.82%   -0.06%     
==========================================
  Files         405      405              
  Lines       53231    53294      +63     
==========================================
+ Hits        46247    46272      +25     
- Misses       6984     7022      +38     

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

tfrederiksen and others added 3 commits January 27, 2025 09:31
- Added typing.
- Allowed lattice to be an alias for the cell block.
- Added check for polarized hamiltonian, still not
  capable of writing anything but un-polarized.
- significantly speeded up the processing step
  before writing the data. There was a lot of
  cleaning the data, which isn't necessary.
- Fixed a bug that could omit the diagonal component
  if the Hamiltonian value was 0.
- streamlined the write process so it is always
  the same supercell connections that gets written.
  The order is still not stringent.

Also added tests for this.

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

zerothi commented Jan 27, 2025

@tfrederiksen I have fixed lots of small problems. With added tests.
Please have a look, once the tests pass, I'll merge!

Thanks for the fixes and report!

Hsub = h[:, i * N : (i + 1) * N]
Hsub.eliminate_zeros()
if not is_orthogonal:
Ssub = S[:, i * N : (i + 1) * N]

Check failure

Code scanning / CodeQL

Potentially uninitialized local variable Error

Local variable 'S' may be used before it is initialized.
Signed-off-by: Nick Papior <[email protected]>
@zerothi zerothi merged commit 74134b4 into zerothi:main Jan 27, 2025
14 of 17 checks passed
@zerothi
Copy link
Owner

zerothi commented Jan 27, 2025

Thanks!

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.

Problems with writing native .ham files
2 participants