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

Add ppafm-conv-rho and ppafm-generate-dftd3. Closes #186 #192

Merged
merged 11 commits into from
Aug 15, 2023

Conversation

mondracek
Copy link
Collaborator

@mondracek mondracek commented Aug 8, 2023

Scripts ppafm-conv-rho and ppafm-generate-dftd3 can be used to call conv_rho.py and generateDFTD3.py, respectively, now. The script example/pyridineDensOverlap/run.sh was updated. Problem of undefined geometry_format in getAtomsWhichTouchPBCcell resolved as testing the pyridineDensOverlap example. Done in the aftermath of and should close #186.

NikoOinonen and others added 9 commits August 4, 2023 10:24
Error message when files with LJ force field not found.
* Introduce script ppafm-conv-rho to call ppafm/cli/conv_rho.py
* Introduce script ppafm-generate-dftd3 to call ppafm/cli/generateDFTD3.py
* Merge branch 'main' into alt-relaxed-scan
* Resolve problems with `generateElFF.py --tip_dens` introduced with the `--input_format` option
* Change "substract" to "subtract" at a few places
Copy link
Collaborator

@ProkopHapala ProkopHapala left a comment

Choose a reason for hiding this comment

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

I assume it works. Otherwise I don't see much for dicussion. Thanks.

@@ -21,63 +22,63 @@ def handleNegativeDensity( rho ):
rho[rho<0] = 0
rho *= ( Q/rho.sum() )

# ======== Main
def main():
Copy link
Collaborator

Choose a reason for hiding this comment

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

So the intend here is to be able call this as a function from another script? Or what is the purpose/advantage to encapsulate everything inside main function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not much about how exactly this works, I just followed the template indicated by @NikoOinonen and used e.g. for ppafm-generate-elff, but... if you look inside the script ppafm-conv-rho, which gets installed by pip based on pyproject.toml, this script actually loads conv_rho.py as from ppafm.cli.conv_rho import main and then calls it by sys.exit(main()). I have no idea if it could be done in a different way.

Copy link
Collaborator

@NikoOinonen NikoOinonen Aug 10, 2023

Choose a reason for hiding this comment

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

There is a feature in setuptools called entry point, which allows easily adding executable scripts to the package distribution. The idea is that you define a function somewhere in your package, add it to the list in pyproject.toml, and then it automatically generate a script upon installation that simply imports and calls that function.

Comment on lines 8 to 13
HELP_MESSAGE = f"""Use this program in the following way:
ppafm-conv-rho -s <filename> -t <filename> [ -A <value> ] [ -B <value> ]
Supported file fromats are:
* xsf
"""

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is needed. You can get a help message for the script by simply calling it with the -h option. E.g. ppafm-conv-rho -h.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright. However, generateElFF.py does have the HELP_MESSAGE constant defined like the above. Is there any reason why generateElFF.py is special? Or should I remove the HELP_MESSAGE from generateElFF.py too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any reason why generateElFF.py would need it either. If you need to print the help message from within the program, you can use the parser.print_help() method. I would remove this from both of the scripts.

@NikoOinonen
Copy link
Collaborator

I checked that it works. Naturally it requires a reinstall for the new scripts.

@NikoOinonen
Copy link
Collaborator

@mondracek I guess we can merge this now, if you don't have any other concerns? I think we should also make a new release after this is merged, so that the examples work correctly on the release version.

@mondracek
Copy link
Collaborator Author

Good, squashing and merging alt-relaxed-scan into main. Regarding the new version, should it be v0.2.2 or something else? Anyway, I will leave it up to @NikoOinonen or others to make the new release version.

@mondracek mondracek merged commit 163ec13 into main Aug 15, 2023
@mondracek mondracek deleted the alt-relaxed-scan branch August 15, 2023 15:28
@NikoOinonen
Copy link
Collaborator

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.

Merge relaxed_scan_PVE.py into relaxed_scan.py?
3 participants