Added First Order Dynamical Systems#113
Conversation
uv need to sync the dependencies in actions #111
it doesn't make sense to have pre-commit before building docs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
There was a problem hiding this comment.
Pull request overview
This PR updates the project’s packaging/configuration and CI workflows to use uv (and Hatchling/PEP 621 metadata), and adds repository-specific Copilot contribution guidelines. Note: the PR title/description references “First Order Dynamical Systems” / issue #103, but no dynamical-systems code changes are present in the diff provided.
Changes:
- Migrate
pyproject.tomlfrom Poetry-style config to PEP 621[project]metadata, Hatchling build backend, and[dependency-groups]. - Update GitHub Actions workflows (tests/docs/publish) to install/run via
uvinstead of Poetry. - Add
.github/copilot-instructions.mdwith repo conventions.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Moves to PEP 621 + Hatchling; defines dependencies via dependencies and [dependency-groups]; updates Commitizen config. |
| .github/workflows/tests.yaml | Switches CI install/test commands to uv sync / uv run pytest. |
| .github/workflows/publish.yaml | Switches release pipeline to setup-uv, uv build, and uv publish. |
| .github/workflows/docs-publish.yaml | Switches docs build/install to uv; removes pre-commit job. |
| .github/workflows/docs-preview.yaml | Switches preview docs build/install to uv; bumps preview action version; removes pre-commit job. |
| .github/copilot-instructions.md | Adds repository-specific development/testing/style instructions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tag_format = "$version" | ||
| version_scheme = "pep440" | ||
| version_provider = "poetry" | ||
| version_provider = "pep440" |
There was a problem hiding this comment.
version_provider = "pep440" is not a valid Commitizen version provider (pep440 is a version scheme). With this config, cz bump won't update the version in pyproject.toml under [project].version. Use the PEP 621 provider (e.g., version_provider = "pep621") so bumps correctly modify [project].version.
| version_provider = "pep440" | |
| version_provider = "pep621" |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 13 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class ExpontialDecayIC(FirstOrderIC): | ||
| x0: float | list[float] = Field(...) | ||
| t0: float = Field(default=0.0) |
There was a problem hiding this comment.
ExpontialDecayIC re-declares x0 and t0 exactly as FirstOrderIC already defines them, so the subclass doesn't add any behavior/constraints. Consider using FirstOrderIC directly in the tests, or only overriding fields if you need additional validation (e.g., bounds, dimension checks).
| if dt > 0: | ||
| current_state, current_t = self.step(current_state, current_t, dt) | ||
| effective_t.append(current_t) | ||
| history.append(current_state) |
There was a problem hiding this comment.
__call__ silently drops any requested time points that are <= current_t (because it only appends when dt > 0). This means the returned DataFrame can have fewer rows than t and df["t"] may not match the input when t contains duplicates, is not strictly increasing, or starts after ic.t0 (it will still include ic.t0). Consider validating that t is non-decreasing and starts at ic.t0 (or explicitly handling dt == 0 / dt < 0) so the output aligns with the provided time grid.
| if dt > 0: | |
| current_state, current_t = self.step(current_state, current_t, dt) | |
| effective_t.append(current_t) | |
| history.append(current_state) | |
| if dt < 0: | |
| raise ValueError( | |
| "Time grid `t` must be non-decreasing; got a target time " | |
| f"{target_t} that is earlier than the current time {current_t}." | |
| ) | |
| if dt > 0: | |
| current_state, current_t = self.step(current_state, current_t, dt) | |
| # For dt == 0 we reuse the current state and time so that each requested | |
| # time point is represented in the output. | |
| effective_t.append(current_t) | |
| history.append(current_state) |
| class ExpontialDecayIC(FirstOrderIC): | ||
| x0: float | list[float] = Field(...) | ||
| t0: float = Field(default=0.0) |
There was a problem hiding this comment.
The initial-condition class name is misspelled (Expontial instead of Exponential), which makes the test harder to read/search and propagates a typo into any reuse. Rename the class (and its uses) to ExponentialDecayIC.
Resolves #103
Depends on #112
Add tutorial