Libxc: 7.0.0 -> 7.1.2 (toolchain); fix external parameter handling#7655
Libxc: 7.0.0 -> 7.1.2 (toolchain); fix external parameter handling#7655Growl1234 wants to merge 8 commits into
Conversation
|
Changes in properties that depend on higher derivatives are expected, since Libxc 7.1.0 fixes many finite precision issues. In specific, exchange functionals are no longer evaluated in the problematic |
Thank you for pointing out! I'd be definitely happy if such deviation is due to issue fixes. That said, one of the concerns is that adapting to the changes will definitely break the testing workflow, as they worked "well" and consistent from libxc v5.1.7 to v7.0.0, and CI currently uses the system-installed libxc from Ubuntu 22.04. Maybe we will have to raise the minimum version requirement to v7.1.0 and then build libxc from the toolchain in CI; fortunately, thanks to the redesign of |
ca50f2b to
7077a64
Compare
|
@susilehtola I tested with ABACUS's internal PBE functional instead in Or, anyone familiar with ABACUS's libxc interface can take a look? |
|
Moreover the libxc unittest fails. MODULE_HAMILT_XCTest_PBEMODULE_HAMILT_XCTest_PZ_SPNMODULE_HAMILT_XCTest_VXC |
|
Thanks you for preparing this update and running the additional tests. It is great to see Libxc 7.1.0 finally released, congratulations to @susilehtola and the Libxc team on the long-awaited numerical and implementation improvements. That said, given the substantial changes in this release and the currently unexplained deviations in ABACUS, switching the default Toolchain and CI to 7.1.0 may be premature. I suggest moving the investigation to a dedicated issue and collecting the relevant test results and discussion there. Until these differences are understood, I would prefer to keep Libxc 7.0.0 as the default and not merge this PR yet. |
I saw 7.1.1 is coming; I will do checks against this branch locally. I agree with filing an issue on the ABACUS side. Edit: The deviation is still present. |
|
I found the issue. Libxc 7.1 added a fourth external parameter, |
So is your fix compatible with both versions? |
In principle it shoud be backward-compatible with previous versions (even the currently minimun-required v5.1.7). |
|
The three deviations in |
Sounds like you are not checking that your parameter arrays are of the right size; please add a proper check to guard from memory corruption. |
|
I think it should also quit when requested_ext_params is shorter than nref. It's dangerous to do "Missing trailing parameters retain Libxc's own defaults". |
Agreed. However, with the current interface, a strict size check would make the Libxc 7.1 tests fail immediately because ABACUS still supplies the old three-parameter PBE array. I think the interface should be redesigned so that functionals specified only by name do not call |
|
The number should be an exact match, since otherwise you don't know what you're going to get... |
Yes, it's quite surprising to see xc_func_set_ext_params called when input is just PBE. |
2b6e6bd to
9df83d0
Compare
|
It seems that there is a change in your XC interface @kirk0830 🤔 Would you mind taking a review at your spare time? Thanks! |
|
@AsTonyshment noted. |
2de32d0 to
3cd21cf
Compare
|
Why is the AI review triggered with every push? |
This update might need careful checks from the ABACUS side, especially regarding
module_xcpath. I detected several "fatal deviation" when running integration tests:CC: @susilehtola, I hope you could have a look if convenient.