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

STM settings as warg to the to_stm method of the PartialCharge class #158

Merged
merged 68 commits into from
Jun 18, 2024

Conversation

MichaelWolloch
Copy link
Contributor

STM settings (including smoothening parameters and interpolation factor for constant current STM) are now settable by the user by passing and STM_settings dataclass to to_stm.

MichaelWolloch and others added 30 commits February 13, 2024 11:54
…_STM

# Conflicts:
#	src/py4vasp/_raw/data.py
#	src/py4vasp/_raw/definition.py
cleaned up plotting a bit. Added unit tests (not for plotting)
…eights over the surface layer instead of arbitrary units.
intended for plotting heatmaps and contour plots
Note that plotly does not show this label at the moment
Got rid of all attributes and put settings in dataclass.

Also added correct py4vasp errors.
Also shift the centers of the points to align with the data
…and fixed the tests that asserted this wrongly.
martin-schlipf and others added 24 commits March 14, 2024 21:32
…owed.

Also made sure that strange fractional coordinates (e.g. -2.234) are correctly mapped between 0 and 1.

For constant current mode, the smoothed charge density is now also rolled in a way that values on both sides of the unit cell boarders are possible.

Tests were updated, but I still need to double check if the STM pictures are actually correct in some edge cases.
…_STM

# Conflicts:
#	core/poetry.lock
#	poetry.lock
…lowest spot, not surface, which is a bit harder to get right for all cases...
@@ -137,6 +147,7 @@ def to_stm(
object.
"""
_raise_error_if_vacuum_too_small(self._estimate_vacuum())
self.stm_settings = stm_settings
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.stm_settings = stm_settings

Please do not mutate classes. If you need to, use this pattern instead

partial_charge_with_new_settings = calc.partial_charge.modify_settings(...)
partial_charge_with_new_settings.to_stm(...) 

I believe that is not necessary at all in this case, you could just do

calc.partial_charge.to_stm(..., new_settings)

without storing the settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I understand correctly that I should pass the stm_settings input along to the other methods as additional input instead of storing it in self.stm_settings?

I thought it would be a good idea to have a stm_settings property, so the user can query the current settings (including the defaults) by
calc.partial_charge.stm_settings

How would I do that if I do not store the settings?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to access the settings that a STM image has been generated with, you should store it in the returned image. I think you should be able to just set an attribute on the Contour class.

graph = calc.partial_charge.to_stm()
graph.series[0].settings

Another alternative that would be perhaps be more modular is to add a meta_data attribute to the Graph class. Then you could use

graph = calc.partial_charge.to_stm()
graph.meta_data["stm_settings"]

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can still get the default settings with the settings property, it is just not a good idea to allow mutating that because it leads to hard to spot errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A complete use case could then look like

from py4vasp import calculation as calc
stm_settings = calc.partial_charge.stm_settings
# ... modify stm settings
graph = calc.partial_charge.to_stm(..., stm_settings)

Comment on lines 328 to 351
def test_stm_updated_settings(PolarizedNonSplitPartialCharge):
defaults = {
"sigma_xy": 4.0,
"sigma_z": 4.0,
"truncate": 3.0,
"enhancement_factor": 1000,
"interpolation_factor": 10,
}
new_settings = {
"sigma_xy": 3.0,
"sigma_z": 5.0,
"truncate": 2.0,
"enhancement_factor": 2000,
"interpolation_factor": 20,
}
settings = PolarizedNonSplitPartialCharge.stm_settings
print(type(settings))
default_settings = dataclasses.asdict(settings)
assert default_settings == defaults
settings = STM_settings(**new_settings)
print(settings)
PolarizedNonSplitPartialCharge.to_stm(stm_settings=settings)
updated_settings = dataclasses.asdict(PolarizedNonSplitPartialCharge.stm_settings)
assert updated_settings == new_settings
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead test whether passing the settings to the to_stm method modifies the resulting plot.

@martin-schlipf martin-schlipf merged commit 661b9ef into vasp-dev:master Jun 18, 2024
27 checks passed
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.

2 participants