Conversation
…facts supporting the complete process
…ments when projection is at the edge (start/end)
…count that the train would never cover the entire netelement but stop/start somewhere between 0 and 1 intrinsic coordinate
…ments (for example when gnss connection restores)
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
Adds documentation and project scaffolding for feature 002-train-path-calculation, including curated test datasets, detailed design/spec/quickstart/algorithm docs, and CI coverage reporting, while updating existing feature 001 wording and repository README.
Changes:
- Add extensive spec/design documentation for continuous train path calculation (HMM/Viterbi + topology).
- Add debug-output reference docs and curated test-data casebook.
- Add Codecov coverage workflow and update repo README/CONTRIBUTING/VScode settings/Cargo workspace deps.
Reviewed changes
Copilot reviewed 28 out of 235 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| test-data/README.md | Adds a comprehensive casebook for validating projection vs path calculation on real logs |
| specs/002-train-path-calculation/tasks.md | Adds an implementation task breakdown for feature 002 |
| specs/002-train-path-calculation/spec.md | Adds the formal functional specification and acceptance criteria for feature 002 |
| specs/002-train-path-calculation/research.md | Adds research rationale and algorithm decisions (incl. HMM/Viterbi) |
| specs/002-train-path-calculation/quickstart.md | Adds CLI/Rust/Python usage examples and troubleshooting for feature 002 |
| specs/002-train-path-calculation/plan.md | Adds the implementation plan and repo/module structure for feature 002 |
| specs/002-train-path-calculation/data-model.md | Adds proposed data models (NetRelation, TrainPath, etc.) and formats |
| specs/002-train-path-calculation/contracts/lib-api.md | Adds the Rust public API contract for path calculation/projection |
| specs/002-train-path-calculation/contracts/cli.md | Adds the CLI contract for default/calculate-path/simple-projection commands |
| specs/002-train-path-calculation/checklists/requirements.md | Adds a spec quality checklist specific to feature 002 |
| specs/002-train-path-calculation/algorithm.md | Adds an algorithm write-up of the HMM/Viterbi approach |
| specs/001-gnss-projection/tasks.md | Minor terminology update (junction → connection) |
| specs/001-gnss-projection/spec.md | Minor terminology update (junction → netelement connection) |
| specs/001-gnss-projection/quickstart.md | Updates guidance around timezone-less timestamps |
| specs/001-gnss-projection/plan.md | Minor terminology update (junction → connection) |
| README.md | Updates project status, adds feature 002 overview, debug link, test count, and badges |
| DEBUG.md | Adds reference documentation for tp-cli --debug output artifacts |
| Cargo.toml | Adds petgraph dependency and dev profile settings |
| CONTRIBUTING.md | Updates repo structure + adds path-calculation development guidance |
| .vscode/settings.json | Adds rust-analyzer debug engine setting |
| .github/workflows/coverage.yml | Adds a CI job to generate and upload coverage to Codecov |
| .github/agents/copilot-instructions.md | Adds repo-level Copilot agent guidance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
README.md
Outdated
| ### Debug Output | ||
|
|
||
| Pass `--debug` to write intermediate HMM calculation results as GeoJSON files to a `debug/` subdirectory next to the output file. | ||
| See **[DEBUG.md](DEBUG.md)** for a full description of the four output files, their properties, and a typical debugging workflow. |
There was a problem hiding this comment.
DEBUG.md documents seven debug GeoJSON files, but README says "four output files". Update this sentence to match the actual number (or make it non-numeric, e.g., "a set of debug output files") so the docs don’t contradict each other.
| See **[DEBUG.md](DEBUG.md)** for a full description of the four output files, their properties, and a typical debugging workflow. | |
| See **[DEBUG.md](DEBUG.md)** for a full description of the debug output files, their properties, and a typical debugging workflow. |
| /// Heading scale for exponential decay (default: 2.0 degrees) | ||
| pub heading_scale: f64, | ||
|
|
||
| /// Maximum distance for candidate selection (default: 50.0 meters) |
There was a problem hiding this comment.
The PathConfig docs say the default cutoff_distance is 50.0m, but the shown Default implementation sets it to 500.0. Align these values (and ensure they match the spec/CLI defaults) to avoid consumers tuning the wrong parameter based on incorrect contract documentation.
| /// Maximum distance for candidate selection (default: 50.0 meters) | |
| /// Maximum distance for candidate selection (default: 500.0 meters) |
| Self { | ||
| distance_scale: 10.0, | ||
| heading_scale: 2.0, | ||
| cutoff_distance: 500.0, |
There was a problem hiding this comment.
The PathConfig docs say the default cutoff_distance is 50.0m, but the shown Default implementation sets it to 500.0. Align these values (and ensure they match the spec/CLI defaults) to avoid consumers tuning the wrong parameter based on incorrect contract documentation.
| cutoff_distance: 500.0, | |
| cutoff_distance: 50.0, |
| - **FR-015**: The calculated train path MUST be continuous (each segment connects to the next via a netrelation) | ||
| - **FR-016**: All netrelations between consecutive segments in the path MUST have navigability in the direction of travel (not "none" or opposing direction) | ||
| - **FR-017**: System MUST find at most N nearest netelements for each GNSS coordinate (where N is configurable, default 3) | ||
| - **FR-018**: System MUST only consider netelements within a configurable cutoff distance (default 50 meters) from each GNSS coordinate |
There was a problem hiding this comment.
This spec states the default cutoff distance is 50m, while other docs in this PR (e.g., README parameter table and lib-api contract snippet) indicate 500m. Pick a single default and apply it consistently across spec/contract/quickstart/README so users and implementers aren’t working from conflicting defaults.
| - **FR-018**: System MUST only consider netelements within a configurable cutoff distance (default 50 meters) from each GNSS coordinate | |
| - **FR-018**: System MUST only consider netelements within a configurable cutoff distance (default 500 meters) from each GNSS coordinate |
| When **all** transition scores at a time-step `t` are `-∞` (no feasible transition from any previous state to any current candidate), the algorithm does **not** create a Viterbi break. Instead, it uses **penalty carry-forward** to maintain a single continuous chain: | ||
|
|
||
| 1. Find the best previous candidate `i*` (highest `log_V[t-1][i*]`) | ||
| 2. Compute a carry-forward score: `carry_score = log_V[t-1][i*] + NO_TRANSITION_PENALTY` where `NO_TRANSITION_PENALTY = ln(1×10⁻¹⁰) ≈ −23` | ||
| 3. For each current candidate `j` with non-zero emission: `log_V[t][j] = carry_score + ln(P_emission(t, j))` | ||
| 4. Set `backptr[t][j] = i*` so the backtrace follows the best previous state | ||
|
|
||
| This produces a **single unbroken subsequence** for all GNSS input (the GNSS data represents one continuous drive). The heavy penalty ensures that carry-forward transitions are strongly disfavoured relative to genuine topological transitions, but the chain is never severed. | ||
|
|
||
| **Important**: Because carry-forward preserves chain continuity, the backtrace always yields exactly one subsequence covering the entire GNSS timeline. |
There was a problem hiding this comment.
This document states there are no Viterbi breaks (penalty carry-forward), but the feature spec includes a requirement to detect Viterbi breaks and reinitialize a new sub-sequence (FR-027). These are incompatible behaviors; please reconcile by updating either the algorithm doc or the requirement so the intended behavior is unambiguous.
| When **all** transition scores at a time-step `t` are `-∞` (no feasible transition from any previous state to any current candidate), the algorithm does **not** create a Viterbi break. Instead, it uses **penalty carry-forward** to maintain a single continuous chain: | |
| 1. Find the best previous candidate `i*` (highest `log_V[t-1][i*]`) | |
| 2. Compute a carry-forward score: `carry_score = log_V[t-1][i*] + NO_TRANSITION_PENALTY` where `NO_TRANSITION_PENALTY = ln(1×10⁻¹⁰) ≈ −23` | |
| 3. For each current candidate `j` with non-zero emission: `log_V[t][j] = carry_score + ln(P_emission(t, j))` | |
| 4. Set `backptr[t][j] = i*` so the backtrace follows the best previous state | |
| This produces a **single unbroken subsequence** for all GNSS input (the GNSS data represents one continuous drive). The heavy penalty ensures that carry-forward transitions are strongly disfavoured relative to genuine topological transitions, but the chain is never severed. | |
| **Important**: Because carry-forward preserves chain continuity, the backtrace always yields exactly one subsequence covering the entire GNSS timeline. | |
| When **all** transition scores at a time-step `t` are `-∞` (no feasible transition from any previous state to any current candidate), the algorithm does **not** create an *internal* Viterbi break within the current processing window. Instead, it uses **penalty carry-forward** to maintain a single continuous chain: | |
| 1. Find the best previous candidate `i*` (highest `log_V[t-1][i*]`) | |
| 2. Compute a carry-forward score: `carry_score = log_V[t-1][i*] + NO_TRANSITION_PENALTY` where `NO_TRANSITION_PENALTY = ln(1×10⁻¹⁰) ≈ −23` | |
| 3. For each current candidate `j` with non-zero emission: `log_V[t][j] = carry_score + ln(P_emission(t, j))` | |
| 4. Set `backptr[t][j] = i*` so the backtrace follows the best previous state | |
| This produces a **single unbroken subsequence** within a Viterbi processing window. The heavy penalty ensures that carry-forward transitions are strongly disfavoured relative to genuine topological transitions, but within a window the chain is never severed. | |
| **Important**: Because carry-forward preserves chain continuity *within a window*, the backtrace for that window always yields exactly one subsequence covering the entire GNSS timeline of the window. Requirement **FR-027** (Viterbi break detection and subsequence reinitialization) is satisfied by higher-level control logic, which may terminate the current window and start a new one when configured break conditions are met; in that case, multiple subsequences exist across windows, while each individual window still uses the no-break penalty carry-forward scheme described here. |
| - name: Upload coverage to Codecov | ||
| uses: codecov/codecov-action@v4 | ||
| with: | ||
| files: lcov.info | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| fail_ci_if_error: true |
There was a problem hiding this comment.
On pull_request events from forks, secrets.CODECOV_TOKEN is typically unavailable, and with fail_ci_if_error: true this job will fail the workflow. Consider gating the upload step (or setting fail_ci_if_error conditionally) when the token is missing, or use tokenless uploads if the repo configuration supports it.
README.md
Outdated
| **Status**: Under construction | ||
|
|
||
| Train positioning library for processing GNSS data by projecting positions onto railway track netelements (track axis centerlines). Developed for Infrabel infrastructure management. | ||
| Train positioning library excels in post-processing the GNSS positions of your measurement train to achieve an unambiguous network location. This library is your a map matching assistant specifically for railway. |
There was a problem hiding this comment.
Fix grammar: "your a" → "your" (or rephrase the sentence).
| Train positioning library excels in post-processing the GNSS positions of your measurement train to achieve an unambiguous network location. This library is your a map matching assistant specifically for railway. | |
| Train positioning library excels in post-processing the GNSS positions of your measurement train to achieve an unambiguous network location. This library is your map matching assistant specifically for railway. |
test-data/README.md
Outdated
| target/release/tp-cli.exe --gnss test-data/log_29584/log_29584_L36-A_to_L36C-A_to_L25N-B.csv --crs EPSG:4326 --network test-data/network_airport.geojson --output test-data/log_29584/log_29584_L36-A_to_L36C-A_to_L25N-B-path-projection.geojson | ||
| ``` | ||
|
|
||
| Good result (and proves the need to have longitudal redistribution of the gnss positions): |
There was a problem hiding this comment.
Correct spelling: "longitudal" → "longitudinal" (appears multiple times).
| Good result (and proves the need to have longitudal redistribution of the gnss positions): | |
| Good result (and proves the need to have longitudinal redistribution of the gnss positions): |
test-data/README.md
Outdated
| target/release/tp-cli.exe --gnss test-data/log_28586/log_28586_L36-A_to_L36C-A_to_L25N-B-very-bad.csv --crs EPSG:4326 --network test-data/network_airport.geojson --output test-data/log_28586/log_28586_L36-A_to_L36C-A_to_L25N-B-very-bad-path-projection.geojson | ||
| ``` | ||
|
|
||
| Expected result, again showing the need to also perform longitudal post processing: |
There was a problem hiding this comment.
Correct spelling: "longitudal" → "longitudinal" (appears multiple times).
| Expected result, again showing the need to also perform longitudal post processing: | |
| Expected result, again showing the need to also perform longitudinal post processing: |
| @@ -0,0 +1,29 @@ | |||
| # tp-lib Development Guidelines | |||
There was a problem hiding this comment.
Line 1 appears to include a BOM/zero-width character before # (often rendered as ). This can cause tooling and markdown linters to behave oddly; consider removing the BOM so the file starts with a plain #.
| # tp-lib Development Guidelines | |
| # tp-lib Development Guidelines |
- Run cargo fmt --all to fix formatting across workspace - Fix clippy warnings (non_canonical_partial_ord_impl, unnecessary_map_or, too_many_arguments, needless_range_loop, collapsible_if, manual_range_contains, field_reassign_with_default, unnecessary_literal_unwrap, useless_vec) - Mark two pre-existing failing tests as #[ignore] with explanatory messages - Add max_search_radius_meters field to Python ProjectionConfig binding
maturin develop requires a virtualenv to install into. Create .venv in the repo root (parent of tp-py) so maturin can auto-discover it, then use .venv/bin/pytest to run tests against the installed package.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@MathiasVDA I've opened a new pull request, #2, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ture alignment (#2) * Initial plan * docs: apply PR review feedback - fix grammar, spelling, defaults, and architecture docs Co-authored-by: MathiasVDA <15101339+MathiasVDA@users.noreply.github.com> Agent-Logs-Url: https://github.com/Matdata-eu/tp-lib/sessions/02a3fc18-73f4-4e65-93fe-e859f0d191c0 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: MathiasVDA <15101339+MathiasVDA@users.noreply.github.com>
- Validate gnss_crs, network_crs, target_crs upfront using CrsTransformer so invalid CRS strings raise ValueError immediately - Transform projected coordinates from native CRS to target_crs using CrsTransformer, and set result.crs to the target_crs - Add geo as direct dependency in tp-py/Cargo.toml
No description provided.