Repository navigation
Use depth dependence and a per-waypoint model for stationkeeping time - #411
Conversation
…ing whether schedule or instr config; use more helper functions
…ependent on depth); add winch_speed to CTD config
…nkeeping_time = deceleration + deployment + acceleration time across codebase
… what MFP assumes
| instrument: InstrumentType | list[InstrumentType] | None = None | ||
| sea_depth_meter: float | None = pydantic.Field( | ||
| default=None, le=0.0 | ||
| ) # estimated depth from MFP, if available. If None, instrument deployment times costed against configured max depth, thus over-estimateing rather than under-estimating than under-estimating. |
There was a problem hiding this comment.
Estimated depths from MFP will follow in #412
| ) -> _InstrumentConfigMixin | None: | ||
| """The configuration for an instrument type, or None if it is not configured.""" | ||
| return getattr(self, self._CONFIG_FIELDS[instrument_type]) | ||
|
|
There was a problem hiding this comment.
Neater way of accessing the configuration for a given instrument
erikvansebille
left a comment
There was a problem hiding this comment.
Looks good - a few comments below
| ``` | ||
|
|
||
| ```{important} | ||
| Remember that MFP's default of 45 minutes per station may not be enough once your instruments are selected. Time at a station is made up of slowing the ship down, carrying out the deployments, and getting back up to speed again. |
There was a problem hiding this comment.
Be more explicit about how the total time is made up of these three aspects. I.e. explicitly mention the default 15+15 minutes of time needed for slow-down and speed-up?
| DEFAULT_TS_CONFIG = {"period_minutes": 5.0} | ||
|
|
||
| # ship config timings, edited in minutes in their own (collapsed by default) section | ||
| SHIP_TIME_ATTRS = ("deceleration_time", "acceleration_time") |
There was a problem hiding this comment.
Maybe it's my physics-brain but when I see deceleration and acceleration, I think in terms of m s-2. Perhaps better to call it 'station_slowdown_time' and 'station_speedup_time'?
There was a problem hiding this comment.
Yes good point, now changed throughout the code for consistency (in e7174b3). Note, I have used "waypoint" instead of "station" throughout, also just to keep it consistent with how we refer to them throughout the code.
|
|
||
| # advanced settings, which most users can leave as the defaults | ||
| with Collapsible( | ||
| title="Ship deceleration & acceleration times", |
There was a problem hiding this comment.
Also make explicit that this is at stations?
| # TODO: docs suggest that add individual instrument stationkeeping times are cumulative, which is at odds with measurements being done simultaneously onboard here | ||
| # TODO: update one or the other? | ||
| return max(time_costs) | ||
| return self._expedition.stationkeeping_time(waypoint) |
There was a problem hiding this comment.
For my information, is this also relevant for ports?
There was a problem hiding this comment.
Ports do not currently get slow down and speed up times, which is perhaps less realistic because of course a ship must still speed up from departure and slow down to arrival... At the moment this means the code is simpler because the Ports can simply be treated as waypoints with no instruments (and therefore no need to slow down/speed up), which sometimes users may specify so that logic would exist anyway. Do you reckon it's worth adding another rule for the Ports?
There was a problem hiding this comment.
No worries, I don't think it's worth the extra code complexity. Let's leave then as is for now
|
|
||
|
|
||
| def register_instrument(instrument_type): | ||
| """Register a concrete instrument class, refusing if doesn't meet the full Instrument contract.""" |
There was a problem hiding this comment.
These words 'contract' and 'concrete' are quite different lingo than we use elsewhere. Rephrase?
There was a problem hiding this comment.
Yes good point, will change
Co-authored-by: Erik van Sebille <erikvansebille@gmail.com>
…for deployment, XBTs do not need to
…els/virtualship into depth-dependent-stationkeeping
|
I have now addressed the comments @erikvansebille. Please note, I have also made a few changes to the docs (quickstart and sail_the_ship guide; in 801b4cf), which go into a bit more detail on the timings associated with each instrument, plus links to the |
erikvansebille
left a comment
There was a problem hiding this comment.
Looks good - only one comment left. Otherwise good to go!
There was a problem hiding this comment.
Does drifters now have the same new sentence twice? o is that a rendering mistake in my git diff?
There was a problem hiding this comment.
Oops that's my mistake, good spot thanks!
Note
This PR is part of a stack of three related PRs (#410, #411, #412). Each one builds on the one before it. I will request reviews for each in turn.
This PR replaces the fixed, per-instrument stationkeeping time approach with a per-waypoint model. The time a waypoint adds to the schedule is now the sum of the ship's deceleration time (to come to a stop), the instrument deployment time, and the ship's acceleration time (to get back up to cruising speed). Waypoints with no instruments add no time, as the ship sails straight past without decelerating/accelerating.
Deceleration and acceleration time fields are added to the
ShipConfigobject in expedition.py (15 minutes each by default) and are intended as ballpark default estimates. Accordingly, they are added as configuration options in thevirtualship plantool.All instruments must now also have a
deployment_timeclassmethod, which determines how much time their deployment adds at the waypoint. This keeps the timing specific and configurable by developers to each instrument. At present, only CTDs add time beyond deceleration and acceleration. A CTD cast's duration depends on the water depth, using a newsea_depth_meterfield on each waypoint and a newwinch_speed_meter_per_secondin the CTD config. Whensea_depth_meteris left asnull, the cast is assumed to reach the CTD's configured maximum depth rather than bathymetry (i.e. the slowest case). Incorporating the estimated sea depth from the MFP exports will be implemented in a subsequent PR (i.e. #412).The total stationkeeping time calculation now lives in one place (
Expedition.stationkeeping_time) and is used by both schedule verification and the simulation, which previously kept separate versions of the logic.Overall, this PR instroduces a breaking change, in that the previous
expedition.yamlfiles with per-instrumentstationkeeping_time_minutesfields will be rejected.Docs and tests have also been updated.
Other note(s)
deployment_timeabstract classmethod, have been moved to the@register_instrumentdecorator. This allows both class attributes (sensor kernels) and classmethods (deployment_time) to be checked at import (with messaging if something is wrong) before runtime/the instrument subclasses are instantiated.Closes #249