Skip to content

fix_setup_etot - #7489

Merged
mohanchen merged 1 commit into
deepmodeling:developfrom
19hello:fix_setup_etot
Jun 19, 2026
Merged

fix_setup_etot#7489
mohanchen merged 1 commit into
deepmodeling:developfrom
19hello:fix_setup_etot

Conversation

@19hello

@19hello 19hello commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Reminder

  • Have you linked an issue with this pull request?
  • Have you added adequate unit tests and/or case tests for your pull request?
  • Have you noticed possible changes of behavior below or in the linked issue?
  • Have you explained the changes of codes in core modules of ESolver, HSolver, ElecState, Hamilt, Operator or Psi? (ignore if not applicable)

Linked Issue

Fix #...

Unit Tests and/or Case Tests for my changes

  • A unit test is added for each new feature or bug fix.

What's changed?

  • Example: My changes might affect the performance of the application under certain conditions, and I have tested the impact on various scenarios...

Any changes of core modules? (ignore if not applicable)

  • Example: I have added a new virtual function in the esolver base class in order to ...

Copilot AI review requested due to automatic review settings June 18, 2026 14:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the setup_etot helpers in the lattice and ion relaxation modules by simplifying their APIs and standardizing how current/previous total energies are tracked, then updates call sites and unit tests accordingly.

Changes:

  • Simplified Lattice_Change_Basic::setup_etot and Ions_Move_Basic::setup_etot signatures (removed judgement and stream parameters) and aligned implementations to a consistent “previous = last current” update rule.
  • Updated relaxation drivers (CG/SD/BFGS paths) to call the new setup_etot APIs.
  • Collapsed/updated unit tests for setup_etot to match the new signatures.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
source/source_relax/test/lattice_change_basic_test.cpp Updates setup_etot unit tests for lattice relaxation after API change.
source/source_relax/test/ions_move_basic_test.cpp Updates setup_etot unit tests for ionic relaxation after API change.
source/source_relax/lattice_change_cg.cpp Adjusts CG lattice relaxation to call the new setup_etot signature.
source/source_relax/lattice_change_basic.h Updates setup_etot declaration (doc needs alignment with new signature).
source/source_relax/lattice_change_basic.cpp Simplifies setup_etot implementation for lattice energy bookkeeping.
source/source_relax/ions_move_sd.cpp Updates SD ionic relaxation to call the new setup_etot signature.
source/source_relax/ions_move_cg.cpp Adjusts CG ionic relaxation to call the new setup_etot signature.
source/source_relax/ions_move_bfgs.cpp Adjusts BFGS ionic relaxation to call the new setup_etot signature.
source/source_relax/ions_move_basic.h Updates setup_etot declaration (doc needs alignment with new signature).
source/source_relax/ions_move_basic.cpp Simplifies setup_etot implementation for ionic energy bookkeeping.
Comments suppressed due to low confidence (2)

source/source_relax/test/lattice_change_basic_test.cpp:490

  • The updated SetupEtot test no longer covers the stress_step == 1 branch (initialization of etot/etot_p), so the behavior introduced/retained in setup_etot() for the first stress step is now untested. This is a regression in unit coverage for setup_etot().
TEST_F(LatticeChangeBasicTest, SetupEtot)
{
    Lattice_Change_Basic::stress_step = 2;
    double energy_in = 80.0;
    std::vector<double> etot_info = {100.0, 90.0};

    Lattice_Change_Basic::setup_etot(energy_in, etot_info);

    EXPECT_DOUBLE_EQ(100.0, etot_info[1]);
    EXPECT_DOUBLE_EQ(80.0, etot_info[0]);
    EXPECT_DOUBLE_EQ(-20.0, etot_info[0] - etot_info[1]);
}

source/source_relax/test/ions_move_basic_test.cpp:285

  • The updated SetupEtot test only covers istep > 1 and no longer exercises the istep == 1 initialization branch in Ions_Move_Basic::setup_etot(). Since that branch is still part of the function’s behavior, it should remain covered by unit tests.
// Test the setup_etot() function
TEST_F(IonsMoveBasicTest, SetupEtot)
{
    // Initialize data
    const int istep = 2;
    std::vector<double> etot_info = {2.0, 1.0};
    double energy_in = 3.0;

    // Call the function being tested
    std::ofstream ofs("/dev/null");
    Ions_Move_Basic::setup_etot(energy_in, istep, etot_info);
    ofs.close();

    // Check the results
    EXPECT_DOUBLE_EQ(etot_info[1], 2.0);
    EXPECT_DOUBLE_EQ(etot_info[0], 3.0);
    EXPECT_DOUBLE_EQ(etot_info[0] - etot_info[1], 1.0);
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread source/source_relax/lattice_change_basic.h
Comment thread source/source_relax/ions_move_basic.h
@mohanchen mohanchen added Refactor Refactor ABACUS codes Bugs Bugs that only solvable with sufficient knowledge of DFT GeometryRelaxation Issues related to geometry relaxation labels Jun 19, 2026

@mohanchen mohanchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@mohanchen
mohanchen merged commit 93454a3 into deepmodeling:develop Jun 19, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bugs Bugs that only solvable with sufficient knowledge of DFT GeometryRelaxation Issues related to geometry relaxation Refactor Refactor ABACUS codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants