Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ABINIT DFPT SHG #1060

Open
wants to merge 160 commits into
base: main
Choose a base branch
from

Conversation

VicTrqt
Copy link
Contributor

@VicTrqt VicTrqt commented Nov 19, 2024

Summary

  • ABINIT workflow to compute the second-harmonic generation (SHG) tensor in DFPT with its test
    • includes DDK, DDE, DTE, mrgddb, and anaddb jobs
  • Output files saving (DDB and POT)
  • Job to remove and compress files from a previous Flow

TODO

  • Add the possibility of a scissor shift

Note

  • This DFPT SHG workflow is mentioned in the draft as being under review. The initial plan was to include it fully in the upcoming paper, but I would understand this might be unfeasible at this stage of the process. Also, since the GW WF is explained in the draft, priority should be given to GW-BSE calculation with Abinit  #816 .
  • The new tests will fail until a new version of Abipy with my changes is released.
  • @gpetretto went over the code for an internal review in [WIP] ABINIT DFPT SHG VicTrqt/atomate2#1

@JaGeo
Copy link
Member

JaGeo commented Nov 25, 2024

@VicTrqt Hi, thank you!

There seem to be some failing abipy imports in the ase part of the tests. Could you check them? It might be that you should only import abipy when abipy is installed.

Copy link
Member

@JaGeo JaGeo left a comment

Choose a reason for hiding this comment

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

I added some initial comments but would need to take a closer look at another point.

@@ -38,7 +38,8 @@ dependencies = [
]

[project.optional-dependencies]
abinit = ["abipy>=0.9.3"]
#abinit = ["abipy>=0.9.3"]
abinit = ["abipy @ git+https://github.com/abinit/abipy.git"] # tmp
Copy link
Member

Choose a reason for hiding this comment

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

Before we merge this, we would definitely need a new abipy version.

dijk: Optional[list] = Field(
None, description="Conventional SHG tensor in pm/V (Chi^(2)/2)"
)
epsinf: Optional[list] = Field(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
epsinf: Optional[list] = Field(
eps_inf: Optional[list] = Field(

?

Comment on lines +32 to +48
# # need to inherit from MSONable to be stored in the data store
# # I tried to combine it with @dataclass, but didn't work...
# class DdbFileStr(MSONable):
# """Object storing the raw string of a DDB file."""

# def __init__(self, ddbfilepath: str | Path, ddb_as_str: str) -> None:
# self.ddbfilepath: str | Path = ddbfilepath
# self.ddb_as_str: str = ddb_as_str

# @classmethod
# def from_ddbfile(cls, ddbfile: DdbFile) -> Self:
# """Create a DdbFileStr object from the native DdbFile abipy object."""
# with open(ddbfile.filepath) as f:
# ddb_as_str = f.read()
# return cls(ddbfilepath=ddbfile.filepath, ddb_as_str=ddb_as_str)


Copy link
Member

Choose a reason for hiding this comment

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

?

Comment on lines +94 to +102
basename=dct_abi_psp["basename"],
type_psp=dct_abi_psp["type"],
symbol=dct_abi_psp["symbol"],
Z=dct_abi_psp["Z"],
Z_val=dct_abi_psp["Z_val"],
l_max=dct_abi_psp["l_max"],
md5=dct_abi_psp["md5"],
filepath=dct_abi_psp["filepath"],
repo_name=repo_name,
Copy link
Member

Choose a reason for hiding this comment

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

I think the naming and the hypens are a bit inconsistent. "basename" vs. "repo_name".

@VicTrqt
Copy link
Contributor Author

VicTrqt commented Nov 26, 2024

Hello @JaGeo !
Thanks for your comments! I'll try addressing them as they come.

There seem to be some failing abipy imports in the ase part of the tests. Could you check them? It might be that you should only import abipy when abipy is installed.

I'm not sure what you mean... Just in case, I modified the required abipy version (temporarily) so that the tests could pass with the github version. Seems that triggered something else with the pseudos so I'll fix that.

@JaGeo
Copy link
Member

JaGeo commented Nov 26, 2024

@VicTrqt the tests never passed. Not even before the modification

@VicTrqt
Copy link
Contributor Author

VicTrqt commented Nov 26, 2024

@JaGeo Yes it was expected since new abipy factories are required and a version of abipy with those new factories has not been released yet.

Note

* The new tests will fail until a new version of Abipy with my changes is released.

@VicTrqt
Copy link
Contributor Author

VicTrqt commented Nov 26, 2024

I was able to fix the tests (still temporarily relying on the github abipy). However, in test_dfpt, abinit is called to generate the perturbations. It could be mocked, but it would not be trivial in terms of work, especially considering future tests... so we decided to install abinit as a dependency so that the tests could pass online. This means that there is now a mixture of mock and real abinit calls. I also added a skip of the test_dfpt in case abinit was not in the PATH so that this test would not be triggered locally by developers without an abinit executable.

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