Skip to content

Conversation

@kubanmar
Copy link
Collaborator

@kubanmar kubanmar commented Sep 4, 2025

In this PR I want to refactor how to deal with not converged or otherwise not finished calculations.

The first step is to refactor and unify how we represent the convergence of the simulation workflow. This PR introduces a new subsection WorkflowConvergenceTarget to all cases where a workflow converges, e.g., SCF, or geometry optimization. It is intended to replace the convergence parameters in GeometryOptimizationModel as well as the convergence settings in SCFOutputs and SelfConsistency.

SCFOutputs is very minimal now and I don't know what it is used for. The information about the SCF should be included in the workflow section. Should SCFOutputs be a reference then, to not duplicate information? Is SCFOutputs used anywhere?

This is a draft PR, I left the failing tests in there on purpose to keep track of the functionality that I broke by removing SelfConsistency.

Note: This PR breaks the abinit parser, because it tries to populate workflow.geometry_optimization.GeometryOptimizationModel.convergence_tolerance_energy_difference, which moved to WorkflowConvergenceTarget

Summary of changes:

  1. add more quantities to Program
  2. add WorkflowConvergenceTarget
  3. add WorkflowConvergenceTarget to SimulationWorkflowModel
  4. remove SelfConsistency
  5. remove self_consistency_ref from PhysicalProperty
  6. remove convergence from SCFOutputs
  7. remove convergence from GeometryOptimizationModel

@kubanmar kubanmar self-assigned this Sep 4, 2025
Copy link
Collaborator

@JFRudzinski JFRudzinski left a comment

Choose a reason for hiding this comment

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

This looks good to me.

TODOs (ideally in the next 2 weeks, i.e., by 27.10):

  • Add SCF as a workflow class, like GeomOpt
  • Determine a standard for populating SCF and SCF+GeomOpt, ideally using a representative parser
  • Align with @ndaelman in the context of #191 and decide what indicators will be present in data.outputs
  • Consider backcompatibility / integration procedure in the parsers

@coveralls
Copy link

coveralls commented Oct 17, 2025

Pull Request Test Coverage Report for Build 18594497000

Details

  • 21 of 21 (100.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 90.259%

Totals Coverage Status
Change from base Build 18342061192: -0.01%
Covered Lines: 5958
Relevant Lines: 6601

💛 - Coveralls

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.

4 participants