Skip to content

Addition of new density-density operators mentioned in the discussion… #53

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

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

Conversation

LeoGaspard
Copy link

Addition of NupNdn, NupNup and NdnNdn operators. Addition of the equivalent hole density-density operators handled at the compile step following the discussion #50.

@awietek
Copy link
Owner

awietek commented Apr 28, 2025

Dear Leo,

thank you very much for the pull request. Currently, according to the automated tests, the MPI code is not compiling: https://github.com/awietek/xdiag/actions/runs/14702408841/job/41256972462?pr=53

Please have a look at it and try to fix it. I will then take a closer look at the new code.

@awietek
Copy link
Owner

awietek commented Apr 29, 2025

Thank you very much, the tests now work. I would have the following two suggestions before proceeding:

  • As we discussed, I would not like to add all the operators you now added to XDiag, but only "NupNup", "NupNdn", "NdnNup", and "NdnNdn". The reason is that I want to replace these correlators later on with an algebra feature and correlations like these operators might then be deprecated. So I do not want to have too many operators in there and I think all the measurements you want to perform can be done with the above four. So that's why I suggested to only implement "NupNup", "NupNdn", "NdnNup" and compile "NupNdn" to "NdnNup". So I would like you to remove the lines when you compile the operators like "NhtotNhtot". For you I think it does not make much difference, you could just have one line of code defining these correlators in your application, is this correct? Sorry if I was not sufficiently clear about that.

  • Second, I would like the operators to be tested in a symmetrized setting as well. This is done in line 48 and following of `tests/algebra/test_apply.cpp". Could you please add the corresponding tests (there only done for "NtotNtot") for the remaining diagonal operators?

Thanks for all your efforts

@LeoGaspard
Copy link
Author

Dear Alexander,

Thank you for your input. Indeed I misunderstood what you told me, I now removed all the composed operators, leaving only NupNdn, NupNup and NdnNdn. I also added the tests in the file test/algebra/test_apply.cpp for the operators NupNdn (both i j and j i since it is important to check that they can be different), NupNup and NdnNdn.

@awietek
Copy link
Owner

awietek commented Apr 29, 2025

Thanks, now there is a test failing unfortunately with the MPI library. Could you have a look into what the problem is there?

@awietek
Copy link
Owner

awietek commented Apr 30, 2025

Thanks, so superficially all looks good. I will have to take a closer look now which can take one or two days, then I'll come back to you. I guess you can already use the version for project.

Copy link
Owner

@awietek awietek left a comment

Choose a reason for hiding this comment

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

Thanks, this is almost ready to be merged. I have two suggestions:

  • The identity operator implementation should be changed, see annotations
  • The NdnNup operator should be added in addition to the NupNdn operator, by compilation.

Copy link
Owner

Choose a reason for hiding this comment

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

This function should be moved to apply_identity.hpp. Also, this is specific to the electron bases. I would implement this more generically, something like this:

void apply_identity(Coupling const &cpl, const coeff_t *vec_in, coeff_t *vec_out, int64_t size) {
    using bit_t = typename basis_t::bit_t;
    coeff_t s = cpl.scalar().as<coeff_t>();
    for (int64_t idx=0; idx<size; ++idx) {
            vec_out[idx] += s * vec_in[idx];
    }
}

which will work for the distributed case, you only have to give the size as an argument.

Copy link
Owner

Choose a reason for hiding this comment

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

Looks good

Copy link
Owner

Choose a reason for hiding this comment

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

looks good

@@ -11,6 +11,7 @@
#include <xdiag/basis/electron_distributed/apply/apply_raise_lower.hpp>
#include <xdiag/basis/electron_distributed/apply/apply_szsz.hpp>
#include <xdiag/basis/electron_distributed/apply/apply_u.hpp>
#include <xdiag/basis/apply_identity_distributed.hpp>
Copy link
Owner

Choose a reason for hiding this comment

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

please follow the suggestions iin apply_identity_distributed.hpp. Move to apply_identity.hpp

electron_distributed::apply_ndn_ndn<coeff_t>(
cpl, op, basis_in, vec_in.memptr(), vec_out.memptr());
} else if (type == "Id") {
apply_identity_distributed<coeff_t>(
Copy link
Owner

Choose a reason for hiding this comment

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

Change call as suggested


inline const std::vector<std::string> real_types = {
"Id", "SdotS", "Exchange", "SzSz", "Sz", "S+",
"S-", "Hop", "Hopup", "Hopdn", "Cdagup", "Cup",
"Cdagdn", "Cdn", "HubbardU", "Ntot", "Nup", "Ndn",
"Nupdn", "NtotNtot", "NupdnNupdn", "tJSzSz", "tJSdotS"};
"Nupdn", "NtotNtot", "NupdnNupdn", "tJSzSz", "tJSdotS",
"NupNdn", "NupNup", "NdnNdn"};
Copy link
Owner

Choose a reason for hiding this comment

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

Add NdnNup here

{"NupNdn", 2},
{"NupNup", 2},
{"NdnNdn", 2},
};
Copy link
Owner

Choose a reason for hiding this comment

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

Add NdnNup here

@@ -25,7 +25,8 @@ void check_valid(Op const &op) try {
} else if ((type == "Hop") || (type == "Hopup") || (type == "Hopdn") ||
(type == "SzSz") || (type == "SdotS") || (type == "Exchange") ||
(type == "NtotNtot") || (type == "NupdnNupdn") ||
(type == "tJSzSz") || (type == "tJSdotS")) {
(type == "tJSzSz") || (type == "tJSdotS") || (type == "NupNdn") ||
(type == "NupNup") || (type == "NdnNdn")) {
Copy link
Owner

Choose a reason for hiding this comment

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

Add NdnNup here

Copy link
Owner

Choose a reason for hiding this comment

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

We should take care of the case that an NdnNup operator gets properly ordered to NupNdn to compare it.

@@ -19,6 +19,7 @@ Generic operators in XDiag are represented as [OpSum](opsum.md) objects made up
| `Nup` | A number operator for an $\uparrow$ spin $n_{i\uparrow}$ | 1 | tJ, Electron, tJDistributed, ElectronDistributed |
| `Ndn` | A number operator for an $\downarrow$ spin $n_{i\downarrow}$ | 1 | tJ, Electron, tJDistributed, ElectronDistributed |
| `Ntot` | A number operator $n_i = n_{i\uparrow} + n_{i\downarrow}$ | 1 | tJ, Electron, tJDistributed, ElectronDistributed |
| `Nhtot` | A number operator for holes $n^H_i = 2 - n_{i\uparrow} - n_{i\downarrow}$ | 1 | tJ, Electron, tJDistributed, ElectronDistributed |
Copy link
Owner

Choose a reason for hiding this comment

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

Remove Nhtot here

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