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

Feature: Converge Thresholds #699

Merged
merged 12 commits into from
Apr 29, 2024
Merged

Feature: Converge Thresholds #699

merged 12 commits into from
Apr 29, 2024

Conversation

AndresOrtegaGuerrero
Copy link
Member

@AndresOrtegaGuerrero AndresOrtegaGuerrero commented Apr 29, 2024

When doing a simulation with a system containing a higher number of atom , the convergence thresholds are quite lose.

For simulations using aiidalab-qe-vibroscopy this is impontant.
image

@AndresOrtegaGuerrero AndresOrtegaGuerrero marked this pull request as draft April 29, 2024 08:15
@AndresOrtegaGuerrero AndresOrtegaGuerrero marked this pull request as ready for review April 29, 2024 09:44
Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

Hi @AndresOrtegaGuerrero , thanks for the work! I made a few small suggestions.

  • The step of scf_conv_thr etc., is better to be similar to the default value.

I already approved the PR. Please make the changes and merge the PR. There is no need for me to review it again.

Comment on lines 297 to 300
num_atoms = 1
if self.input_structure is not None:
ase_structure = self.input_structure.get_ase()
num_atoms = ase_structure.get_global_number_of_atoms()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
num_atoms = 1
if self.input_structure is not None:
ase_structure = self.input_structure.get_ase()
num_atoms = ase_structure.get_global_number_of_atoms()
num_atoms = len(self.input_structure.sites) if self.input_structure else 1

@@ -357,7 +441,7 @@ def set_metallic_magnetization(self, parameters):

def set_panel_value(self, parameters):
"""Set the panel value from the given parameters."""

# protocol_parameters = PwBaseWorkChain.get_protocol_inputs(protocol)
Copy link
Member

Choose a reason for hiding this comment

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

delete this line?

self.scf_conv_thr = ipw.BoundedFloatText(
min=1e-15,
max=1.0,
step=0.000001,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
step=0.000001,
step=1e-10,,

self.forc_conv_thr = ipw.BoundedFloatText(
min=1e-15,
max=1.0,
step=0.00001,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
step=0.00001,
step=0.0001,

@superstar54
Copy link
Member

The user may want to change the value by mouse; in the case of SCF conv., when I click the up arrow, I assume the user wants to increase from 8e-10 to 9e-10, but it increases too much. As shown here:

qeapp-conv.mp4

@AndresOrtegaGuerrero
Copy link
Member Author

The user may want to change the value by mouse; in the case of SCF conv., when I click the up arrow, I assume the user wants to increase from 8e-10 to 9e-10, but it increases too much. As shown here:

qeapp-conv.mp4

I did the changes you suggested and it helps on this, however the issue is that the default values depend on the structure selected, since aiida-quantumespresso plugin set this based on the number of atoms of the structure , so for big systems the step of the widget will affect this differently ...

@AndresOrtegaGuerrero
Copy link
Member Author

The user may want to change the value by mouse; in the case of SCF conv., when I click the up arrow, I assume the user wants to increase from 8e-10 to 9e-10, but it increases too much. As shown here:
qeapp-conv.mp4

I did the changes you suggested and it helps on this, however the issue is that the default values depend on the structure selected, since aiida-quantumespresso plugin set this based on the number of atoms of the structure , so for big systems the step of the widget will affect this differently ... I will try if i can modify the step after setting the structure

@superstar54
Copy link
Member

however the issue is that the default values depend on the structure

Good point! Anyway, we provide a small value so the user can control it. Otherwise, it's impossible for the user to control it using a mouse.

@AndresOrtegaGuerrero
Copy link
Member Author

however the issue is that the default values depend on the structure

Good point! Anyway, we provide a small value so the user can control it. Otherwise, it's impossible for the user to control it using a mouse.

I have an idea, just let me try something

Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

LGTM!

@AndresOrtegaGuerrero AndresOrtegaGuerrero merged commit da51afd into main Apr 29, 2024
18 checks passed
@AndresOrtegaGuerrero AndresOrtegaGuerrero deleted the feature/conv_forces branch April 29, 2024 14:20
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