-
Notifications
You must be signed in to change notification settings - Fork 10
UI for schedule and ship config building #193
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
Conversation
…; unify exception raising in ScheduleEditor and ConfigEditor.
…d error message handling in ScheduleEditor.
… `virtualship init` user messaging
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.
Very nice, @j-atkins! I played around a bit and it worked quite smooth! A few minor user-oriented comments below
| self.config.adcp_config | ||
| and self.config.adcp_config.max_depth_meter == -1000.0 | ||
| ) | ||
| yield Label(" OceanObserver:") |
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.
DO we need to explain a bit more what these instruments are? Or where to find more information about them?
| "title": "Drifter", | ||
| "attributes": [ | ||
| {"name": "depth_meter"}, | ||
| {"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.
Make clear in the tool that lifetime is in minutes? And why not days, like Argo?
| await pilot.click(save_button) | ||
| await pilot.pause(0.5) | ||
|
|
||
| args, _ = plan_screen.notify.call_args |
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.
The (expected) error I see now is that there is an error saving changes, each waypoint should be after another. But would it help users to notify at which waypoint the error happens?
src/virtualship/cli/_plan.py
Outdated
| UNEXPECTED_MSG_ONSAVE = ( | ||
| "Please ensure that:\n" | ||
| "\n1) All typed entries are valid (all boxes in all sections must have green borders and no warnings).\n" | ||
| "\n2) Time selections exist for all waypoints.\n" |
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 there also a way to delete waypoints in the tool?
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.
(or add)
VeckoTheGecko
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.
Just a couple points of clarification re. data model and UI. I haven't done an in depth review of the code itself in _plan.py (since this is an isolated part of the codebase, and I'm not familiar with the conventions used in textualised code) - but have briefly looked through it and I think its good :)
src/virtualship/models/location.py
Outdated
| if self.lon > 360: | ||
| raise ValueError("Longitude cannot be larger than 360.") | ||
| if self.lon > 180: | ||
| raise ValueError("Longitude cannot be larger than 180.") |
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.
I think that this should be between -180 to 360 (i.e., in line with copernicusmarine)until we decide within virtualship exactly how we are handling crossing the international date line
src/virtualship/cli/_plan.py
Outdated
| UNEXPECTED_MSG_ONSAVE = ( | ||
| "Please ensure that:\n" | ||
| "\n1) All typed entries are valid (all boxes in all sections must have green borders and no warnings).\n" | ||
| "\n2) Time selections exist for all waypoints.\n" |
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.
(or add)
|
Thanks for the reviews - here are the requested changes! In summary:
In addition:
|
|
I've been encountering a bug with adding and removing waypoints where it crashes if you quickly press remove waypoints. (to recreate you can quickly add a bunch, and quickly remove a bunch). Do you know what could be the cause of that @j-atkins ? |
Good spot! I'll look into it. |
|
Latest commit should now catch and avoid the |
Here's a version of the new UI tool for interactive schedule and ship config building. Inevitably there will be bits that can be improved, so I look forward to the feedback. See GIF at the bottom for quick look at the UI in action!
This is quite a large PR (apologies...) but I'll give an overview the new proposed workflow next. And then also some notes on the UI's features and some notes/considerations.
Proposed workflow
This PR adds a new (optional)
virtualship plancommand to the CLI. This command is to be used aftervirtualship init.So, the workflow is now something like:
virtualship init [EXPEDITION_DIR]to generate the 'schedule.yaml' and 'ship_config.yaml' files.virtualship plan [EXPEDITION_DIR]reads in the newly generated YAMLs, allows the user to modify values in both (including waypoint locations, timings and instrument selections) before writing the updated entries to the YAMLs on pressing the save button.virtualship fetch [EXPEDITION_DIR]as before.virtualship run [EXPEDITION_DIR]as before.UI features
schedule.verify()methods are run upon pressing save, checking that the scheduling is valid.UserErrorandUnexpectedErrorclasses). Depending on the error, users will be notified by messages within the UI or in the terminal log if the error is caught on launch. In cases where the error is unexpected, users are prompted to raise an issue on the VirtualShip issue tracker.Notes
textualandpytest-asynciodependencies added.virtualship initwith the--from-mfpoption, this will no longer configure the instrument choices from the MFP export file. This therefore removes the previous method where users would add an extraInstrumentcolumn in the Excel file. This step is now handled in the UI.start_timeandend_timein 'ship_config.yaml' are set up to be autofilled if they are left blank in the UI (given they fall under the "advanced users only" section in the UI). It's not immediately clear to me whatend_timeshould automatically default to. I have currently set it to take the time of the last waypoint + 42 days to cover the default drifter lifetime in case a drifter is selected in the last waypoint. However, I also notice that the drifter data download infetchadds a 21-day buffer. I'm not sure where it's best to handle this.Quite a lot of text there but let me know if anything's unclear!
P.S. GIF!