feat(model): #35 add an interface of integrals of motion #54
Conversation
Coverage Report (73a702c, 3.12, ubuntu-latest)
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report (73a702c, 3.11, macos-latest)
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report (73a702c, 3.12, macos-latest)
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
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"]There was a problem hiding this comment.
I propose to separate engineering and scientific efforts to different issues and MRs, so we can streamline iterations.
| ], | ||
| ) | ||
| def test_output( | ||
| self, omega: float, input: dict[str, float], expected: dict[str, float] |
There was a problem hiding this comment.
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.
| @@ -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.
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
| 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.
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.
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.
Note that
Let us sleep over it a bit and then decide.
There was a problem hiding this comment.
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
cmp0xff
deleted the
feature/yw/35-add-an-alternative-initial-condition-for-undamped-simple-harmonic-oscillator
branch
cmp0xff
restored the
feature/yw/35-add-an-alternative-initial-condition-for-undamped-simple-harmonic-oscillator
branch
cmp0xff
changed the title
No description provided.