Skip to content

Conversation

@BariumOxide13716
Copy link
Collaborator

@BariumOxide13716 BariumOxide13716 commented Apr 1, 2025

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)
    Due to an unexpected behavior of running parallelly with 2 cores as reported in outlying behavior when running with 2 cores for mixing kinetic energy density #6086, the integrate test 218 is deleted such that it does not use mixed kinetic energy density (calculations using which yield a different result with 2 cores compared with those with a different number of cores) during the test calculation.

Linked Issue

Concerned with #6086 but not yet resolved.

Unit Tests and/or Case Tests for my changes

N.A.

What's changed?

Integrate test 218 is removed.

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

N.A.

@BariumOxide13716 BariumOxide13716 added Bugs Bugs that only solvable with sufficient knowledge of DFT Interfaces Interfaces with other packages labels Apr 1, 2025
@BariumOxide13716 BariumOxide13716 requested a review from dyzheng April 1, 2025 06:57
@mohanchen
Copy link
Collaborator

Do we have tests that use mixed kinetic energy density?

@mohanchen
Copy link
Collaborator

Sorry, I don't understand why we need to remove a test when we find a bug?

@BariumOxide13716
Copy link
Collaborator Author

Sorry, I don't understand why we need to remove a test when we find a bug?

Sorry for the confusion. The test that revealed this bug is likely to fail in the CI/CD. Because we have submitted an issue relating to this bug, we will try to fix this bug as we resolve the issue, and add the test back.

And to respond to the previous question on whether we had a test on mixed kinetic energy density, the test was not there until 4 days ago. @dyzheng was unable to make this test pass when he tried to merge from the develop branch. So now the plan is to remove the test, so the develop branch can be merged to other people's branches, meanwhile we will try to fix this bug.

@mohanchen
Copy link
Collaborator

Make sense, thanks for the explanation.

@mohanchen
Copy link
Collaborator

I found there is a conflict, should we fix it before merging the PR?

@BariumOxide13716
Copy link
Collaborator Author

I found there is a conflict, should we fix it before merging the PR?

Definitely. I'll update you when this conflict is resolved tmr. (Sorry for the delay.)

@mohanchen
Copy link
Collaborator

I found there is a conflict, should we fix it before merging the PR?

Definitely. I'll update you when this conflict is resolved tmr. (Sorry for the delay.)

OK, thanks.

@BariumOxide13716 BariumOxide13716 removed the request for review from dyzheng April 2, 2025 10:14
@BariumOxide13716
Copy link
Collaborator Author

I found there is a conflict, should we fix it before merging the PR?

Definitely. I'll update you when this conflict is resolved tmr. (Sorry for the delay.)

OK, thanks.

The code should be in good shape and ready to merge now. Please feel free to approve and merge it anytime.

@dyzheng dyzheng merged commit dce058a into deepmodeling:develop Apr 3, 2025
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 Interfaces Interfaces with other packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants