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

Refactor of LatticeSlice and Contacts #234

Merged
merged 8 commits into from
Jan 22, 2024
Merged

Refactor of LatticeSlice and Contacts #234

merged 8 commits into from
Jan 22, 2024

Conversation

pablosanjose
Copy link
Owner

@pablosanjose pablosanjose commented Jan 18, 2024

This addresses the first part of the proposal in #232:

  • Make LatticeSlice a general lattice slice object that can refer either to sites (SiteSlice), orbitals (OrbitalSlice), or orbitals grouped by sites (OrbitalSliceGrouped)
  • Make CellIndices a general cell slice object that can also refer to either of the above
  • Make LatticeSlice organize its different CellIndices with a Dictionary for efficient iteration and access
  • Make SiteSlice, OrbitalSlice, OrbitalSliceGrouped, CellSites, CellOrbitals, etc simply aliases of the above objects
  • Prepare the path towards OrbitalSliceArrays by basing SelfEnergys, Contacts, GreenFunctions etc on OrbitalSliceGrouped instead of SiteSlices.

The refactor has been far more tricky that expected, since it touches the internals of all Green solvers. The result, however, is a cleaner and conceptually clearer codebase, I think.

No new tests have been added for the moment, but some additional testing would be advisable.

WIP

contact_subcells_latslice & combine

SiteLike + iterators

selfenergy model fixed

fixing site_to_orbs

orbsdict -> cellorbsdict

rename subcells to cellsdict

sites_to_orbs_grouped cleanup

working on inverse_green_mat

fixed reordered_site_orbitals

g runs

small fix

cleanup

faspaths in sites_to_orbs_grouped

fixing tests

fixing tests

fixing tests

fixing tests

fixing tests

fixing tests

fix tests

fixing tests

fixing orbranges/groups mess

fix tests

fix tests
@codecov-commenter
Copy link

codecov-commenter commented Jan 19, 2024

Codecov Report

Attention: 37 lines in your changes are missing coverage. Please review.

Comparison is base (91af232) 92.40% compared to head (a7b3a18) 92.78%.

Files Patch % Lines
src/types.jl 88.69% 13 Missing ⚠️
src/slices.jl 90.32% 12 Missing ⚠️
src/greenfunction.jl 91.17% 6 Missing ⚠️
src/solvers/green/bands.jl 77.77% 4 Missing ⚠️
src/iterators.jl 88.88% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #234      +/-   ##
==========================================
+ Coverage   92.40%   92.78%   +0.37%     
==========================================
  Files          34       34              
  Lines        5477     5500      +23     
==========================================
+ Hits         5061     5103      +42     
+ Misses        416      397      -19     

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

@pablosanjose
Copy link
Owner Author

pablosanjose commented Jan 19, 2024

Mmm, so that was unexpected. The test failure on Linux was completely unrelated to the PR, but still worrying. It was produced by the FunctionWrapper inside AppliedEigenSolver. Said FunctionWrapper is a closure over a Hamiltonian which somehow was garbaged collected after exiting the apply function that creates the AppliedEigenSolver. This should absolutely never happen, so it seems FunctionWrappers is somehow not registering the closure root with Julia's GC properly. That's at least my current understanding.

The workaround is to call the wrapped eigensolver function once before exiting apply. This seems to solve the problem, but I definitely don't know why.

In any case, this is a completely unrelated issue, see #235

@pablosanjose
Copy link
Owner Author

The use of Dictionaries.jl required some methods that were missing, but they have now been added to that repo (andyferris/Dictionaries.jl#130), and a release has been tagged, so we no longer need the workaround.

@pablosanjose pablosanjose merged commit 0270d5e into master Jan 22, 2024
8 checks passed
@pablosanjose pablosanjose deleted the CellIndices branch January 22, 2024 14:40
@pablosanjose pablosanjose mentioned this pull request Apr 25, 2024
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