Skip to content

Conversation

@YuleZhang936
Copy link

This PR is to merge a noncollinear functional ensemble NCXC programmed by Xiaoyu Zhang into ABACUS. NCXC is based on the multi-collinear approach published on PHYSICAL REVIEW RESEARCH 5, 013036 (2023). This ensemble can currently support LDA and GGA and can be elaborated to mGGA and hybrid functional in the future.
Xiaoyu Zhang

#include <utility>

// 2x2 complex matrix
using Matrix2x2 = std::array<std::array<std::complex<double>, 2>, 2>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add namespace for your code

Copy link
Author

Choose a reason for hiding this comment

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

NCLibxc serves as an independent package and it is not designed to have a namespace. However, it can also be added due to the specific program. If you think that it is necessary, you can conduct this small modification so that it fits your requirements.

Copy link
Author

Choose a reason for hiding this comment

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

namespace has been added

std::ofstream log_file("NCLibxc.log", std::ios::out | std::ios::app);
if (log_file.is_open())
{
log_file << "You are using the multi-collinear approach implemented by Xiaoyu Zhang. Please cite:\n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

which paper should be site?

Copy link
Author

Choose a reason for hiding this comment

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

This paper hasn't been well prepared. This log is only for tests. We are going to add citation requests after papers published.

if (PARAM.inp.nspin == 4 && PARAM.inp.multicolin)
{ // noncollinear case added by Xiaoyu Zhang, Peking University, 2024.10.02. multicollinear method for lda Since NCLibxc needs libxc, this part codes will not be used.

std::cerr << "Error: Multi-collinear approach does not support running without Libxc." << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use WARNING_QUIT function to quit the program.

Copy link
Author

Choose a reason for hiding this comment

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

I am not familiar with the function in ABACUS. If you think that it is necessary, you can conduct this small modification so that it fits your requirements.

Copy link
Author

Choose a reason for hiding this comment

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

This problem is fixed

#ifdef USE_LIBXC
#include "NCLibxc/NCLibxc.h"
#include "xc_functional_libxc.h"
#include <tuple>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you add so many "include" but not use them?

Copy link
Author

Choose a reason for hiding this comment

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

It is for future development of this package and more flexible interfaces. We don't submit those parts to ABACUS. If they are annoying, it is OK to delete them.

Copy link
Author

Choose a reason for hiding this comment

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

I inspect all of them and did some modification.

Copy link
Author

Choose a reason for hiding this comment

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

As for the problem you mentioned, I didn't modify xc_functional_vxc from the very beginning.

Copy link
Collaborator

@dyzheng dyzheng left a comment

Choose a reason for hiding this comment

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

Test should be added for this new method!

Copy link
Collaborator

@WHUweiqingzhou WHUweiqingzhou left a comment

Choose a reason for hiding this comment

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

Please add unit tests and integrated tests. Current code coverage is zero.

@mohanchen mohanchen added the Features Needed The features are indeed needed, and developers should have sophisticated knowledge label Nov 22, 2024
@YuleZhang936
Copy link
Author

I have uploaded a new version of NCXC and implement it into ABACUS. Now it supports lda and gga.

@YuleZhang936
Copy link
Author

Test should be added for this new method!

please refer to benchmark_tests.cpp

@YuleZhang936
Copy link
Author

Please add unit tests and integrated tests. Current code coverage is zero.

could you be more specific?

@YuleZhang936
Copy link
Author

I have uploaded a new version of NCXC and implement it into ABACUS. Now it supports lda and gga. Local tests are successful.

@mohanchen
Copy link
Collaborator

Before merge, we need to discuss how to maintain such as large amount of new codes, which are mostly written by machine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how to maintain this file?

Copy link
Author

Choose a reason for hiding this comment

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

I think that situations are very similar with maintaining libxc. ABACUS provides and maintains interface to NCXC and the owner of NCXC will maintain NCXC on its github page. NCXC serves as an external program like libxc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how to maintain this file?

Copy link
Author

Choose a reason for hiding this comment

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

Please see the above reply.

@mohanchen
Copy link
Collaborator

@YuleZhang936 and I decied to move this part outside of ABACUS and set up a new repository. Therefore, I will close this PR.

@mohanchen mohanchen closed this Mar 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Features Needed The features are indeed needed, and developers should have sophisticated knowledge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants