Repository navigation
Ingest sea depth from MFP export - #412
Conversation
…ing whether schedule or instr config; use more helper functions
…f the waypoint's sea_depth_meter field
…ependent on depth); add winch_speed to CTD config
…nkeeping_time = deceleration + deployment + acceleration time across codebase
… what MFP assumes
…s and is confusing to user)
Co-authored-by: Erik van Sebille <erikvansebille@gmail.com>
…for deployment, XBTs do not need to
…els/virtualship into depth-dependent-stationkeeping
erikvansebille
left a comment
erikvansebille
left a comment
There was a problem hiding this comment.
Looks good - two comments below. Both may be beyond scope, though
| - **Time (`time`)**: The scheduled time for reaching the waypoint, specifically in YYYY-MM-DD HH:MM:SS format. | ||
|
|
||
| - **Sea depth (`sea_depth_meter`)**: The estimated depth of the water column at the waypoint. This value matters because it determines how a long a deployment takes if there is a depth dependence, e.g. for a CTD that has to be lowered and raised. If you leave it as `null`, VirtualShip doesn't know how deep the water is and assumes the CTD descends all the way to its configured `max_depth_meter`. That is the slowest possible cast, so your schedule is verified against a worst case rather than an optimistic one in the simulations. | ||
| - **Sea depth (`sea_depth_meter`)**: The estimated depth of the water column at the waypoint. This value matters because it determines how a long a deployment takes if there is a depth dependence, e.g. for a CTD that has to be lowered and raised. If you leave it as `null`, VirtualShip doesn't know how deep the water is and assumes the CTD descends all the way to its configured `max_depth_meter`. That is the slowest possible cast, so your schedule is verified against a worst case rather than an optimistic one in the simulations. It will be automatically filled in by `virtualship init` if you use the `--from-mfp` flag. |
There was a problem hiding this comment.
I find null a bit of a strange name here. I can imagine it may confuse users, who think that null == zero. Can we not change the code so users can leave it empty? Or otherwise use max?
There was a problem hiding this comment.
My hesitation for using max_depth_meter is that that's only relevant when using CTDs at that waypoint.
Perhaps defaulting tosea_depth_meter being hidden in the expedition.yaml unless using --from-mfp would indeed be best though, to avoid confusion.
There was a problem hiding this comment.
I guess there's no way to also test 'live' export from MFP (in case they change something on their portal)?
There was a problem hiding this comment.
Hmm from a bit of scoping out, this would be quite an involved feature with quite a lot of custom logic for scraping the right things (which will be very fragile to changes on the MFP site itself). In that sense I do think it's out of scope of this PR and would also require quite a lot of maintenance.
The tests I've added for now should at least flag that something is wrong if the export changes.
|
The latest changes mean On a related note, I realise that making I think this is acceptable for now. Someone would have to dig fairly deep into the code to work it out, and the field isn't exposed in user-facing features like |
erikvansebille
left a comment
erikvansebille
left a comment
There was a problem hiding this comment.
Looks good. Two questions below
| - **Time (`time`)**: The scheduled time for reaching the waypoint, specifically in YYYY-MM-DD HH:MM:SS format. | ||
|
|
||
| - **Sea depth (`sea_depth_meter`)**: The estimated depth of the water column at the waypoint. This value matters because it determines how a long a deployment takes if there is a depth dependence, e.g. for a CTD that has to be lowered and raised. If you leave it as `null`, VirtualShip doesn't know how deep the water is and assumes the CTD descends all the way to its configured `max_depth_meter`. That is the slowest possible cast, so your schedule is verified against a worst case rather than an optimistic one in the simulations. It will be automatically filled in by `virtualship init` if you use the `--from-mfp` flag. | ||
| - **Sea depth (`sea_depth_meter`, optional)**: The estimated water depth at the waypoint, as a negative number of metres (e.g. `-1500.0`). It is filled in automatically by `virtualship init --from-mfp`, and can otherwise be added by hand. It is only used to estimate how long depth-dependent deployments (e.g. CTD casts) hold the ship at the waypoint. If it is left out, these are assumed to reach the instrument's configured maximum depth, so time taken at shallow-water waypoints could be overestimated if the configured max depth exceeds the actual depth. The simulated measurements are unaffected either way because the instruments will find and use the actual depth at simulation time. |
There was a problem hiding this comment.
'left out' may be confusing. Do you mean 'left empty'? Or 'not provided'?
| if wp_data.get("sea_depth_meter") is None: | ||
| wp_data.pop("sea_depth_meter", None) |
There was a problem hiding this comment.
Is this first line (if-statement) needed? Is the whole idea of pop not that the second argument is used if it doesn't exist? I.e. can we not just use the second line here?
There was a problem hiding this comment.
I think we need the if-statement because using pop alone will remove sea_depth_meter in all cases, even when it's supposed to be there (e.g. after using --from-mfp).
That said, this has highlighted that the None second argument in pop is largely redundant here because sea_depth_meter should always be present in the Expedition model, even if set to None. I will change it to a version which assumes it is always present and errors out if not, which is probably what we want.
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 implements logic so that
virtualship init --from-mfpnow fillssea_depth_meterfrom the MFP export's "Sea Depth" column.Also updates docs and adds tests.