search

dipc-cc / hubbard

Python tools for mean-field Hubbard models
https://dipc-cc.github.io/hubbard/
GNU Lesser General Public License v3.0
21 stars 8 forks source link

mnt: changed to LinearMixer in non-iteration code #126

Closed zerothi closed 2 years ago

zerothi commented 2 years ago

In the iterate function the mixer could never be re-used. Hence it made no sense to have a history mixer, changed to linearmixer

sofiasanz commented 2 years ago

Hi Nick, thanks for the PR. I have done a small test looking at the time that it takes to converge and the number of iterations needed with one and the other mixer and I do see a difference. For instance, for the same molecule I got this output:

sisl.mixing.PulayMixer(0.7, history=7) -> (t: 0.32595348358154297 s, 33 iterations)
sisl.mixing.LinearMixer(weight=0.7) -> (t: 1.0543715953826904 s, 112 iterations)
zerothi commented 2 years ago

Hi Nick, thanks for the PR. I have done a small test looking at the time that it takes to converge and the number of iterations needed with one and the other mixer and I do see a difference. For instance, for the same molecule I got this output:


sisl.mixing.PulayMixer(0.7, history=7) -> (t: 0.32595348358154297 s, 33 iterations)
sisl.mixing.LinearMixer(weight=0.7) -> (t: 1.0543715953826904 s, 112 iterations)

But I think you are passing a mixer from outside, right? The point with this PR is that here the iterate function should not default a mixer since it won't be returned and there is no history in it. So its value is not important at all!

I would probably change these functions all together. I would say the iterate method should not do the mixing, that should be done outside. But that is a somewhat different matter.

sofiasanz commented 2 years ago

But I think you are passing a mixer from outside, right? The point with this PR is that here the iterate function should not default a mixer since it won't be returned and there is no history in it. So its value is not important at all!

I would probably change these functions all together. I would say the iterate method should not do the mixing, that should be done outside. But that is a somewhat different matter.

OK I see it now! In the converge function it does make a change but in iterate it just doesn't matter. Then yes, I think this PR is OK!

sofiasanz commented 2 years ago

I agree with you that this is not the best approach. I think that this goes back to your comments on the implementation of these functions (https://github.com/dipc-cc/hubbard/issues/119#issuecomment-1049220334), something that we should do at some point, but for the moment I will just merge this PR :-)