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.

Sign up for GitHub

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

Jump to bottom

Implement PpafmParameters class based on pydantic.BaseModel #275

Merged
merged 23 commits into from
Aug 30, 2024
Merged

Implement PpafmParameters class based on pydantic.BaseModel #275

merged 23 commits into from
Aug 30, 2024

Conversation

yakutovicha
Copy link
Collaborator

@yakutovicha yakutovicha commented Apr 9, 2024 •
edited
Loading

fixes #232

  • When the PR is about to be merged, update the defaults.
  • Add tests for the PpafmParameters dataclass.

@ondrejkrejci ondrejkrejci mentioned this pull request Apr 10, 2024
Open
@ondrejkrejci
Copy link
Collaborator

ondrejkrejci commented Aug 20, 2024 •
edited
Loading

@yakutovicha - after the implementation - the note on the default/params.ini on this wiki should be removed.

@yakutovicha
Copy link
Collaborator Author

@NikoOinonen, I guess I can't avoid modifying AFMulator as it depends on the functions that were migrated already. Should I proceed?

@NikoOinonen
Copy link
Collaborator

@yakutovicha Sure, go ahead if it is necessary. Once you are done, I will take a look and maybe make a follow up PR if something should be changed.

@yakutovicha yakutovicha force-pushed the improve/store-config-in-one-class branch from 7af71d7 to 490595a Compare August 21, 2024 19:17
@yakutovicha yakutovicha force-pushed the improve/store-config-in-one-class branch from 490595a to 2e55280 Compare August 21, 2024 19:20
@yakutovicha yakutovicha force-pushed the improve/store-config-in-one-class branch from 0c523bf to a7ad91d Compare August 21, 2024 19:33
@yakutovicha yakutovicha marked this pull request as ready for review August 21, 2024 20:14
@yakutovicha
Copy link
Collaborator Author

I believe this is done. Since the change is quite intrusive, I would like several people to take a look at this PR. Please try to break it and share an example of a simulation that was working before and doesn't work now.

NikoOinonen
NikoOinonen reviewed Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@NikoOinonen NikoOinonen left a comment

Choose a reason for hiding this comment

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

I didn't test properly yet, but I looked through the changes. I did not see anything wrong, but I left a few comments on things that maybe could be changed.

ppafm/common.py Outdated Show resolved Hide resolved
ppafm/common.py Show resolved Hide resolved
ppafm/common.py Outdated Show resolved Hide resolved
ppafm/common.py Outdated Show resolved Hide resolved
ppafm/common.py Outdated Show resolved Hide resolved
ppafm/common.py Outdated Show resolved Hide resolved
@yakutovicha yakutovicha force-pushed the improve/store-config-in-one-class branch from 04ed3ca to 1f1e6f8 Compare August 27, 2024 13:45
@mondracek
Copy link
Collaborator

ppafm-plot-results fails with the --atoms option inside common.parseAtoms, which it calls in

ppafm/ppafm/cli/plot_results.py

Line 120 in 1f1e6f8

i_zs, r_s, _ = common.parseAtoms(atoms, elem_dict, autogeom=False, PBC=parameters.PBC)

Seems that passing the parameters by
i_zs, r_s, _ = common.parseAtoms(atoms, elem_dict, autogeom=False, PBC=parameters.PBC, parameters=parameters)
(adding the last argument parameters=parameters) would help.

@yakutovicha yakutovicha force-pushed the improve/store-config-in-one-class branch from 6bfe128 to e847f99 Compare August 27, 2024 14:21
@yakutovicha yakutovicha force-pushed the improve/store-config-in-one-class branch from e847f99 to 100c68f Compare August 27, 2024 14:24
NikoOinonen
NikoOinonen requested changes Aug 28, 2024
View reviewed changes
ppafm/common.py Outdated Show resolved Hide resolved
ppafm/common.py Outdated Show resolved Hide resolved
@yakutovicha
Copy link
Collaborator Author

Seems that passing the parameters by
i_zs, r_s, _ = common.parseAtoms(atoms, elem_dict, autogeom=False, PBC=parameters.PBC, parameters=parameters)
(adding the last argument parameters=parameters) would help.

Thanks, done in 2242312

yakutovicha
yakutovicha commented Aug 30, 2024
View reviewed changes
ppafm/common.py Outdated
@@ -67,7 +67,6 @@ class PpafmParameters(pydantic.BaseModel):
colorscale_kpfm: str = "seismic"
ddisp: float = 0.05
aMorse: float = -1.6
tip_base: typing.List = ["None", 0.00]
Copy link
Collaborator Author

@yakutovicha yakutovicha Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I had to do it, cause Toml was getting crazy trying to parse an inhomogeneous array.

At the same time, the parameter is not used anywhere (checked with get grep tip_base) and was supposed to be removed.

@yakutovicha
Copy link
Collaborator Author

yakutovicha commented Aug 30, 2024 •
edited
Loading

I did all I was planning to do, so from my side the PR is ready.

NikoOinonen
NikoOinonen approved these changes Aug 30, 2024
View reviewed changes
Copy link
Collaborator

@NikoOinonen NikoOinonen left a comment

Choose a reason for hiding this comment

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

Everything looks good to me.

@yakutovicha yakutovicha merged commit b86d105 into main Aug 30, 2024
13 checks passed
@yakutovicha yakutovicha deleted the improve/store-config-in-one-class branch August 30, 2024 14:07
This was referenced Aug 30, 2024
Open
Open
@NikoOinonen NikoOinonen mentioned this pull request Sep 20, 2024
Merged
NikoOinonen added a commit that referenced this pull request Sep 23, 2024
@NikoOinonen
Fix API docs (#305)
0f18cad 
The API docs were blank because the new dependencies from #275 were missing in the ReadTheDocs build.

The badge on the front page was not showing the failure. This is because the failed imports are treated as warnings instead of errors. So I also added fail_on_warning: true to the config and fixed all of the remaining warnings so that there is at least some indication that the build is failing if this happens again in the future.
yakutovicha added a commit that referenced this pull request Dec 10, 2024
@yakutovicha
Fix issues in ppafm-generate-elff when run with tip's density (#316)
2d8c126 
This PR addresses an issue likely introduced in #275. To prevent regressions, a new example
was added to the test suite to ensure this behaviour is now tested.

During testing, additional issues were identified:
* One issue is handling input arguments, which is fixed in this PR.
* Another issue is reported separately in #321.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NikoOinonen NikoOinonen NikoOinonen approved these changes

@mondracek mondracek Awaiting requested review from mondracek

@ondrejkrejci ondrejkrejci Awaiting requested review from ondrejkrejci

@ProkopHapala ProkopHapala Awaiting requested review from ProkopHapala

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create a class to manage simulation inputs
5 participants
@yakutovicha @ondrejkrejci @NikoOinonen @mondracek @ProkopHapala