SelfConsistentHubbardWorkChain: Relabeling of HubbardStructure if relaxations are skipped
#63
Conversation
The `current_hubbard_structure` is not relabeled in `SelfConsistentHubbardWorkChain` if `skip_relax_iterations != 0`. This can cause problems in the subsequent `PwRelaxWorkChains` if new types are introduced. Therefore, the relabeling step is moved to a separate method that is called in any case.
There was a problem hiding this comment.
Thanks a lot @t-reents ! As a general feedback, I would try to have methods that do specifc thing, and not to mix e.g. checks multiple times. On the other hand, I recognize that it's a bit hard to devise something smart enough, or at least with the current design. For instance, the should_check_convergence emits report messages, but it's called twice, so not the best solution. I think we can live with that, and also it's not said in general that if the meta_convergence is switched on, you would immediately restart a new cycle. With the new HubbardStructureData, actually we can still compare parameters if they have the same length but different kinds.
Co-authored-by: Lorenzo <[email protected]>
t-reents
force-pushed
the
fix/61_relabeling_when_skip_relax
branch
from
214f499
to
1bcc183
Compare
|
Thanks for your comments @bastonero! I agree that this simplified logic makes more sense and that the messages are probably not necessary. I only introduced this logic to keep the previous reports etc., and to not mess things up too much. But as you don't think they are necessary as well, I'm more than happy to remove them. |
|
Hi @t-reents , did you use this feature lately? Does everything work? If yes, I guess we could merge this then. |
|
Hi @bastonero, I used it after I implemented it and everything seems to work, so I think it can be merged |
|
Thanks a lot @t-reents ! |
Fixes #61.
@bastonero I moved the relabeling to a separate method, as discussed. I decided to introduce a new method in
SelfConsistentHubbardWorkChain, instead of a new utility function in a separate module, to directly access the context of theWorkChain. The relabeling logic is the same as before and the method takes also care of generating an appropriate message to be passed toself.report.In this new version, the
HubbardStructurewould also be relabeled ifmeta_convergence == False. If you would like to stick to the current logic, in the sense, that it's not updated, I could simply add anif-statement to the method. This is up to you.The name of the method is of course debatable as well. I wasn't a 100% happy with the current name, as it also checks whether an update is necessary and might not relabel the structure. However, something like
should_relabel...would indicate that aboolis returned. Moreover,update_hubbard...seemed a bit too general.Tests have been adjusted as well and I added another one, to check if the correct reporting messages are provided.