-
Notifications
You must be signed in to change notification settings - Fork 20
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
STM settings as warg to the to_stm method of the PartialCharge class #158
Conversation
…_STM # Conflicts: # src/py4vasp/_raw/data.py # src/py4vasp/_raw/definition.py
…_.py in calculation.
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.
…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...
…rguments Co-authored-by: Martin Schlipf <[email protected]>
Co-authored-by: Martin Schlipf <[email protected]>
…tings can be changed by the user.
@@ -137,6 +147,7 @@ def to_stm( | |||
object. | |||
""" | |||
_raise_error_if_vacuum_too_small(self._estimate_vacuum()) | |||
self.stm_settings = stm_settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"]
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
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 |
There was a problem hiding this comment.
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.
… current mode. still need to attach settings to graph object.
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.