-
Notifications
You must be signed in to change notification settings - Fork 10
Remove spacetime-region #264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ade2622 to
15848f3
Compare
113a3c0 to
ba3793b
Compare
erikvansebille
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Great improvement which will reduce annoying outofbounds errors and thereby increase authenticity!
A few comments below
src/virtualship/cli/_plan.py
Outdated
| {"name": "cycle_days"}, | ||
| {"name": "drift_days"}, | ||
| {"name": "stationkeeping_time", "minutes": True}, | ||
| {"name": "lifetime", "minutes": True}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what "minutes' here means, but would this not be more logical in days?
src/virtualship/instruments/base.py
Outdated
| minimum_longitude=self.min_lon - latlon_buffer | ||
| if spatial_constraint | ||
| else None, | ||
| maximum_longitude=self.max_lon + latlon_buffer | ||
| if spatial_constraint | ||
| else None, | ||
| minimum_latitude=self.min_lat - latlon_buffer | ||
| if spatial_constraint | ||
| else None, | ||
| maximum_latitude=self.max_lat + latlon_buffer | ||
| if spatial_constraint | ||
| else None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct python code? Looks strange, without indents etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes agree it doesn't look great, ruff auto-formatted to this. I'll adjust it anyway 👍
src/virtualship/models/expedition.py
Outdated
|
|
||
| @pydantic.field_serializer("lifetime") | ||
| def _serialize_lifetime(self, value: timedelta, _info): | ||
| return value.total_seconds() / 60.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, would this not make more sense in days?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice that users don't need to set a pace_time_region anymore!
| min_depth_meter: 0.0 | ||
| vertical_speed_meter_per_second: -0.1 | ||
| stationkeeping_time_minutes: 20.0 | ||
| lifetime_minutes: 90720.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in days?
| schedule_start=datetime.datetime(1995, 6, 1, 0, 0, 0), | ||
| schedule_end=datetime.datetime(1995, 6, 30, 0, 0, 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps not relevant for this PR< but at some point it might be good to transition to numpy.datimetime64 throughout the code in order to better align with Parcels v4?
Overview
This PR removes
SpaceTimeRegionfrom the expedition config. Now, instead, space-time boundaries are defined in reference to the extremes of the waypoints when building fieldsets (i.e. the minimum/maximum latitude/longitude/time across all waypoints). This avoids divergence between user defined waypoints and the prescribed space-time region (i.e. #259).I also experimented with not defining any spatial limits at all for the fieldset generation. For instruments which require data at depth (Argos, CTDs, XBTs, ADCP) this is still too slow/memory intensive, so I have kept the lat, lon subsetting in
copernicusmarine.open_dataset(..). However, for surface-only instruments (Drifters), the lat, lon subsetting can be relaxed. This is a big advantage because it removes out-of-bounds issues for Drifters. The bathymetry fields also require no lat, lon bounds.Otherwise of interest - and happily - I think there could a speed boost as result of the changes in this PR (with e.g. Argo floats and the underway instruments), but I haven't confirmed this nor diagnosed exactly why this might be happening.
Drifter/Argo lifetimes
Drifters
InstrumentConfig) also controls the endtime of the fieldset (so it will be the last waypoint time + the drifter lifetime).Argo floats
InstrumentConfig), giving more control to the user. This defaults to 3 months.SpaceTimeRegionproblems when changing waypoints after initialisation #259