HubbardStructureData reach by StructureBrowserWidget #718
Conversation
for more information, see https://pre-commit.ci
|
Hi @AndresOrtegaGuerrero is this ready for review, or it's a draft? |
|
@superstar54 This is a draft , i am testing it , but we have to change the logic of the workchain ( i am on it). |
…dalab/aiidalab-qe into mod/structure_browser_widget
for more information, see https://pre-commit.ci
|
@superstar54 Now is ready, please let me know what you think of the logic? Basically if you put an HubbardStructureData and you dont define new the option DFT+U in advanced tab, the workchain will just create a new StructureData (No Hubbard Parameters) from your HubbardStructureData, and run. If you define new parameter in the advanced tab, it will create a new HubbardStructureData with the new parameters. |
There was a problem hiding this comment.
Hi @AndresOrtegaGuerrero, I understand your logic in the workchain: If a user doesn't specify Hubbard parameters in the GUI, the workchain will use pure StructureData.
Maybe you could also think in another way: if a structure is generated by geometry optimization using certain Hubbard parameters. Then, it's better to do the following calculation using the same Hubbard parameters, which means it is better to pass the HubbardStructureData directly to the builder, to make the results are more consistent.
If you want to keep the logic, we can make the GUI load the Hubbard parameters from the structure if it's a HubbardStructureData.
You don't need to change it in this PR, but we can discuss it in the next relesae.
…dalab/aiidalab-qe into mod/structure_browser_widget
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
@superstar54 I did the changes you suggested, so now if you select a HubbardStructureData, the app will set your u values in the widget and activate override so in case you continue you will be using the same u parameters. In the workchain if the hubbard parameters set are exactly the same as in your structure, there wont be a new HubbardStructureData, but instead the user will do calculations on the same structure. Have a look |
| if hubbard_structure.hubbard == structure.hubbard: | ||
| builder.structure = structure | ||
| else: | ||
| hubbard_structure.store() | ||
| builder.structure = hubbard_structure |
There was a problem hiding this comment.
Raise error when structure is a normal StructureData, which doesn't have hubbard. I think you don't need check the hubbard here, because you need to use the newly created hubbard_structure.
| if hubbard_structure.hubbard == structure.hubbard: | |
| builder.structure = structure | |
| else: | |
| hubbard_structure.store() | |
| builder.structure = hubbard_structure | |
| hubbard_structure.store() | |
| builder.structure = hubbard_structure |
…dalab/aiidalab-qe into mod/structure_browser_widget
for more information, see https://pre-commit.ci
|
@superstar54 Should i add this logic in a function ? outside the class, so it is not no long the logic of the class ? |
| should_store_and_assign = ( | ||
| not isinstance(structure, HubbardStructureData) | ||
| or hubbard_structure.hubbard != structure.hubbard | ||
| ) | ||
|
|
There was a problem hiding this comment.
I don't understand why we need to check hubbard_structure.hubbard != structure.hubbard?
There was a problem hiding this comment.
because if the user is setting the same hubbard parameters, then i do not store the HubbardStructureData, and just give the builder the same structure, otherwise i will create a copy of the same HubbardStructureData. But there is also the other case where the user uses a HubbardStructureData, but decided to change the U parameters, then i should create a new structure and store it.
| # Common logic to store and assign the new structure if conditions are met | ||
| if should_store_and_assign and "hubbard_structure" in locals(): | ||
| hubbard_structure.store() | ||
| builder.structure = hubbard_structure | ||
|
|
There was a problem hiding this comment.
one more question. why not move to line 156?
for more information, see https://pre-commit.ci
| # Initialize flag to determine when to store hubbard_structure | ||
| should_store_and_assign = False | ||
|
|
||
| if not isinstance(structure, HubbardStructureData): | ||
| # Store the HubbardStructureData is structure is not a HubbardStructureData | ||
| should_store_and_assign = True | ||
| else: | ||
| # Check if hubbard parameters are different from the structure | ||
| if hubbard_structure.hubbard != structure.hubbard: | ||
| should_store_and_assign = True | ||
| else: | ||
| # If criteria is not met, assign the structure to the builder | ||
| builder.structure = structure | ||
|
|
||
| # Common logic to store and assign hubbard_structure if conditions are met | ||
| if should_store_and_assign: | ||
| hubbard_structure.store() | ||
| builder.structure = hubbard_structure |
There was a problem hiding this comment.
this logic can be simplified.
| # Initialize flag to determine when to store hubbard_structure | |
| should_store_and_assign = False | |
| if not isinstance(structure, HubbardStructureData): | |
| # Store the HubbardStructureData is structure is not a HubbardStructureData | |
| should_store_and_assign = True | |
| else: | |
| # Check if hubbard parameters are different from the structure | |
| if hubbard_structure.hubbard != structure.hubbard: | |
| should_store_and_assign = True | |
| else: | |
| # If criteria is not met, assign the structure to the builder | |
| builder.structure = structure | |
| # Common logic to store and assign hubbard_structure if conditions are met | |
| if should_store_and_assign: | |
| hubbard_structure.store() | |
| builder.structure = hubbard_structure | |
| # If this is a new HubbardStructureData or the hubbard is different | |
| if not isinstance(structure, HubbardStructureData) or hubbard_structure.hubbard != structure.hubbard: | |
| hubbard_structure.store() | |
| builder.structure = hubbard_structure | |
| else: | |
| builder.structure = structure |
for more information, see https://pre-commit.ci
This addresses #716