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

AiiDAEnsemble: add missing self.init() #335

Merged
merged 1 commit into from
May 14, 2024

Conversation

bastonero
Copy link
Contributor

Fixes #334

The version update v1.4.1 missed to add the new init function to the compute_ensemble method of the auxiliary class AiiDAEnsemble. The simple fix makes the calculation using this class comparable to the traditional Ensemble class.

Fixes SSCHAcode#334

The version update v1.4.1 missed to add the new `init` function to the
`compute_ensemble` method of the auxiliary class `AiiDAEnsemble`.
The simple fix makes the calculation using this class comparable to
the traditional `Ensemble` class.
Copy link
Collaborator

@diegomartinez2 diegomartinez2 left a comment

Choose a reason for hiding this comment

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

Hi, I do not see the "init" function in 'compute_ensemble'. Why not use a __init__ method to automatically initialize the object?, that way you don't need to call a self.init().

@@ -100,6 +100,7 @@ def compute_ensemble(
self.stress_computed = copy(self.force_computed)

self._clean_runs()
self.init()
Copy link
Contributor Author

@bastonero bastonero May 14, 2024

Choose a reason for hiding this comment

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

Hi @diegomartinez2 , thanks for reviewing the code. As you cans see, the function is called here, at the end of the compute_ensemble method. This mimics the behaviour of the Ensemble class. The AiiDAEnsemble in facts subclasses the Ensemble one, hence it is provided with this method. This function (def init()), according to documentation, must be called after the calculation of the ensemble, not to initialize the object itself.

@mesonepigreco
Copy link
Collaborator

@diegomartinez2
The new version has a different def init(self) that initializes the variables for the calculation of the Gradient in fourier space.
This initialization perform the fourier transform of the displacements and the forces. In this case, it is necessary to have already generated both the displacements and computed the forces before doing the initialization of the fourier calculation.
This is the reason why it is required to run self.init() (which is not the init used to create the ensemble) after the forces and displacements of the ensemble have been computed.

We have this in Ensemble.compute_ensemble

if timer:
      timer.execute_timed_function(self.init)
else:
      self.init()

Since the AiiDA Ensemble is overwriting the compute_ensemble, by missing the self.init there prevents the correct initialization of the fourier gradient calculation (and consequently the wrong minimization).

@mesonepigreco mesonepigreco added this to the 1.4.2 milestone May 14, 2024
@mesonepigreco
Copy link
Collaborator

We could use this patch for 1.4.2 since a long time is probably required for 1.5, and we have tons of small fixes to tdscha and cell constructor that have occurred since the last release.

@mesonepigreco mesonepigreco self-requested a review May 14, 2024 12:50
Copy link
Collaborator

@mesonepigreco mesonepigreco left a comment

Choose a reason for hiding this comment

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

The edit is minimal and the testsuite is passing. It should be merged without problems

@mesonepigreco mesonepigreco merged commit 08129da into SSCHAcode:master May 14, 2024
1 check passed
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.

AiiDAEnsemble: missing the new self.init() after the calculation of the ensemble
3 participants