-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat(model): #35 add an interface of integrals of motion #54
base: main
Are you sure you want to change the base?
Changes from all commits
b555f85
be219f5
5800c84
f983f01
73a702c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,11 @@ | |
from numpy.typing import ArrayLike | ||
from pydantic import BaseModel, computed_field, field_validator | ||
|
||
from hamilflow.models.harmonic_oscillator.initial_conditions import ( | ||
HarmonicOscillatorIC, | ||
parse_ic_for_sho, | ||
) | ||
|
||
|
||
class HarmonicOscillatorSystem(BaseModel): | ||
"""The params for the harmonic oscillator | ||
|
@@ -54,19 +59,6 @@ def check_zeta_non_negative(cls, v: float) -> float: | |
return v | ||
|
||
|
||
class HarmonicOscillatorIC(BaseModel): | ||
"""The initial condition for a harmonic oscillator | ||
|
||
:cvar x0: the initial displacement | ||
:cvar v0: the initial velocity | ||
:cvar phi: initial phase | ||
""" | ||
|
||
x0: float = 1.0 | ||
v0: float = 0.0 | ||
phi: float = 0.0 | ||
|
||
|
||
class HarmonicOscillatorBase(ABC): | ||
r"""Base class to generate time series data | ||
for a [harmonic oscillator](https://en.wikipedia.org/wiki/Harmonic_oscillator). | ||
|
@@ -155,6 +147,7 @@ def __init__( | |
system: Dict[str, float], | ||
initial_condition: Optional[Dict[str, float]] = {}, | ||
): | ||
initial_condition = parse_ic_for_sho(system["omega"], **initial_condition) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest I am not happy with the syntax |
||
super().__init__(system, initial_condition) | ||
if self.system.type != "simple": | ||
raise ValueError( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import math | ||
from typing import Any, Dict, cast | ||
|
||
from pydantic import BaseModel, Field | ||
|
||
|
||
class HarmonicOscillatorIC(BaseModel): | ||
"""The initial condition for a harmonic oscillator | ||
|
||
:cvar x0: initial displacement | ||
:cvar v0: initial velocity | ||
:cvar phi: initial phase | ||
""" | ||
|
||
x0: float = Field(default=1.0) | ||
v0: float = Field(default=0.0) | ||
phi: float = Field(default=0.0) | ||
|
||
|
||
def parse_ic_for_sho(omega: float, **kwargs: Any) -> Dict[str, float]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one looks quite bad. Can we reduce the complexity by settle on one certain kind of convention? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't aim to generate a good interface for users to use. As long as we can set the convention for ourselves, I am okay with anyone of the three choices. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that Let us sleep over it a bit and then decide. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can we just keep |
||
"Support alternative initial conditions" | ||
match keys := {*kwargs.keys()}: | ||
case set() if keys <= {"x0", "v0", "phi"}: | ||
ret = {str(k): float(v) for k, v in kwargs.items()} | ||
case set() if keys == {"x0", "t0"}: | ||
ret = dict(x0=float(kwargs["x0"]), v0=0.0, phi=-float(omega * kwargs["t0"])) | ||
case set() if keys == {"E", "t0"}: | ||
ene = cast(float, kwargs["E"]) | ||
ret = dict( | ||
x0=math.sqrt(2 * ene) / omega, v0=0.0, phi=-float(omega * kwargs["t0"]) | ||
) | ||
case _: | ||
raise ValueError( | ||
f"Unsupported variable names as an initial condition: {keys}" | ||
) | ||
if phi := ret.get("phi"): | ||
ret["phi"] = phi % (2 * math.pi) | ||
|
||
return ret |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import math | ||
|
||
import pytest | ||
|
||
from hamilflow.models.harmonic_oscillator.initial_conditions import parse_ic_for_sho | ||
|
||
|
||
@pytest.fixture() | ||
def omega() -> float: | ||
return 2 * math.pi | ||
|
||
|
||
class TestParseICForSHO: | ||
@pytest.mark.parametrize( | ||
("input", "expected"), | ||
[ | ||
( | ||
dict(x0=1.0, v0=1.0, phi=7.0), | ||
dict(x0=1.0, v0=1.0, phi=7 % (2 * math.pi)), | ||
), | ||
(dict(x0=1.0, t0=1.0), dict(x0=1.0, v0=0.0, phi=0.0)), | ||
( | ||
dict(E=1.0, t0=1.0), | ||
dict(x0=math.sqrt(2.0) / (2 * math.pi), v0=0.0, phi=0.0), | ||
), | ||
], | ||
) | ||
def test_output( | ||
self, omega: float, input: dict[str, float], expected: dict[str, float] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now I see what is not compatible with py39... It is |
||
) -> None: | ||
assert parse_ic_for_sho(omega, **input) == expected | ||
|
||
def test_raise(self, omega: float) -> None: | ||
with pytest.raises(ValueError): | ||
parse_ic_for_sho(omega, **dict(x0=1.0, E=2.0)) |
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 we are moving initial conditions to a dedicated module, let's also move the following lines to a dedicated module (maybe
model.py
or something).I feel most people won't look into
__init__.py
unless it is related to__all__
.Maybe we only do imports in
__init__.py
. Something likeThere 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.
I propose to separate engineering and scientific efforts to different issues and MRs, so we can streamline iterations.
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.
More concretely, I propose to merge #56 first, then this #54, and finally the newly created #57.