Add support for subordinate stations#176
Conversation
fc25938 to
b0b1332
Compare
59d168b to
a0b1a90
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #176 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 11 11
Lines 491 505 +14
Branches 44 54 +10
=========================================
+ Hits 491 505 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds support for subordinate tide stations, which don't have their own harmonic constituent data but instead use offsets (time and height) from a reference station to calculate predictions. The implementation allows for both fixed and ratio-based height offsets.
Key Changes
- Adds subordinate station support by looking up reference station data (datums and harmonic constituents)
- Implements flexible offset system supporting both fixed (addition) and ratio (multiplication) height adjustments
- Adds custom Vitest matcher
toBeWithinfor testing predictions with tolerance ranges
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/neaps/src/index.ts | Implements subordinate station logic by fetching reference station data and passing offsets to predictions; blocks unsupported operations (timeline/water level) for subordinate stations |
| packages/tide-predictor/src/harmonics/prediction.ts | Refactors offset interface (renamed fields) and adds support for fixed vs ratio height offset types |
| packages/tide-predictor/src/index.ts | Updates type exports to include ExtremeOffsets |
| packages/tide-predictor/README.md | Updates documentation to reflect renamed offset fields (height_offset → height, time_offset → time) |
| packages/neaps/test/index.test.ts | Adds comprehensive tests for subordinate stations including validation against NOAA data |
| packages/tide-predictor/test/harmonics/prediction.test.ts | Refactors offset tests to use explicit offset objects instead of mock file |
| test/setup.ts | Adds custom toBeWithin matcher for tolerance-based comparisons |
| vitest.config.ts, packages/*/vitest.config.ts | Configures test setup files |
| packages/neaps/package.json | Updates dependency from @neaps/tide-stations to @neaps/tide-database |
Comments suppressed due to low confidence (2)
packages/tide-predictor/test/harmonics/prediction.test.ts:158
- Only ratio offsets are tested, but the implementation supports both 'fixed' and 'ratio' types. Add a test case for fixed offsets to ensure that behavior is properly validated. This would test the code path at lines 78-79 and 85-86 in packages/tide-predictor/src/harmonics/prediction.ts.
it('it can add ratio offsets to secondary stations', () => {
const offsets: ExtremeOffsets = {
height: {
type: 'ratio',
high: 1.1,
low: 1.2
},
time: {
high: 1,
low: 2
}
}
const offsetResults = harmonics({
harmonicConstituents: mockHarmonicConstituents,
phaseKey: 'phase_GMT',
offset: false
})
.setTimeSpan(startDate, extremesEndDate)
.prediction()
.getExtremesPrediction({ offsets })
offsetResults.forEach((offsetResult, index) => {
if (offsetResult.low) {
expect(offsetResult.level).toBeCloseTo(
regularResults[index].level * offsets.height!.low!,
4
)
expect(offsetResult.time.getTime()).toBe(
regularResults[index].time.getTime() + offsets.time!.low! * 60 * 1000
)
}
if (offsetResult.high) {
expect(offsetResult.level).toBeCloseTo(
regularResults[index].level * offsets.height!.high!,
4
)
expect(offsetResult.time.getTime()).toBe(
regularResults[index].time.getTime() + offsets.time!.high! * 60 * 1000
)
}
})
})
packages/tide-predictor/README.md:187
- The documentation doesn't mention the new 'type' property that can be set to 'fixed' or 'ratio' for height offsets. The example shows numeric values for high and low, but doesn't explain that behavior differs based on the type property (addition for 'fixed' vs multiplication for 'ratio'). Consider adding documentation about this property and its effects.
- `height` - **object** - An object of height offets, in the same units as the reference station.
- `high` - **float** - The offset to be added to high tide (can be negative)
- `low` - **float** - The offset to be added to low tide (can be negative)
- `time` - **object** - An object of time offets, in number of minutes
- `high` - **float** - The number of minutes to add to high tide times (can be negative)
- `low` - **float** - The number of minutes to add to low tide times (can be negative)
{
height: {
high: 1,
low: 2
},
time: {
high: 1,
low: 2
}
}
</details>
---
💡 <a href="/neaps/neaps/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
| if (station.type === 'subordinate') { | ||
| reference = findStation(station.offsets?.reference || '') | ||
| } | ||
| const { datums, harmonic_constituents } = reference |
There was a problem hiding this comment.
Inefficient lookup of reference station. The findStation function returns a fully wrapped station object (with all methods), but only the datums and harmonic_constituents properties are needed. Consider extracting these properties directly from the raw station data before wrapping to avoid unnecessary computation and potential deep recursion if reference stations chain together.
| // If subordinate station, use the reference station for datums and constituents | ||
| let reference = station | ||
| if (station.type === 'subordinate') { | ||
| reference = findStation(station.offsets?.reference || '') |
There was a problem hiding this comment.
When a subordinate station's reference is missing or undefined, this defaults to an empty string which will cause findStation to throw a "Station not found: " error. Consider providing a more specific error message that indicates the subordinate station is missing a reference station ID.
| reference = findStation(station.offsets?.reference || '') | |
| const referenceId = station.offsets?.reference | |
| if (!referenceId) { | |
| throw new Error( | |
| `Subordinate station ${station.id} is missing a reference station ID.` | |
| ) | |
| } | |
| reference = findStation(referenceId) |
Paired with openwatersio/tide-database#24, this adds support for getting predictions from subordinate stations by using offsets.