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

feat(model): #35 add an interface of integrals of motion #54

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link
Member

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 like

from hamilflow.models.harmonic_oscillator.model import SimpleHarmonicOscillator, DampdHamonicOscillator


__all__ = ["SimpleHarmonicOscillator", "DampdHamonicOscillator"]

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 propose to separate engineering and scientific efforts to different issues and MRs, so we can streamline iterations.

Copy link
Collaborator Author

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.

Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be honest I am not happy with the syntax system["omega"]. It breaks the design and steals data from the system dictionary. I guess we need to improve the design at some point. @emptymalei

super().__init__(system, initial_condition)
if self.system.type != "simple":
raise ValueError(
Expand Down
39 changes: 39 additions & 0 deletions hamilflow/models/harmonic_oscillator/initial_conditions.py
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]:
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

@emptymalei emptymalei Jun 17, 2024

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that $(E, t_0)$ is associated with the first integral of the system. First integrals, or constants of motion, have special and physical meaning in the framework of analytical mechanics. This is in contrast with the phase $\phi$.

Let us sleep over it a bit and then decide.

Copy link
Member

@emptymalei emptymalei Jun 17, 2024

Choose a reason for hiding this comment

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

Note that (E,t0) is associated with the first integral of the system. First integrals, or constants of motion, have special and physical meaning in the framework of analytical mechanics. This is in contrast with the phase ϕ.

Let us sleep over it a bit and then decide.

Can we just keep $(E, t_0)$ and drop the others? I am still not sure how you are gonna use it. But I am okay with any of these choices. But the last thing I want is all of them clustered in a function. It would be easier to just chose one and force it.

"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
103 changes: 102 additions & 1 deletion poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ plotly = "^5.19.0"

[tool.poetry.group.dev.dependencies]
commitizen = "*"
pre-commit = "*"


[tool.commitizen]
Expand Down
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]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now I see what is not compatible with py39... It is expected: dict[str, float]. Should be expected: Dict[str, float], where Dict is imported from typing.

) -> 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))
Loading