Add support for phased recipe format #98
Conversation
| yield (rospy.get_time(), RECIPE_START.name, self.id) | ||
| for stage in self.stages: | ||
| stage_duration = days_to_seconds(stage["days"]) | ||
| end_of_stage = (rospy.get_time() - self.start_time) + stage_duration |
There was a problem hiding this comment.
I'm probably just being pedantic here, but this computes the end time for the current stage relative to the current time instead of relative to when the previous stage was supposed to end. I believe the latter semantics would be more "correct". So basically I'm suggesting initializing end_of_stage to self.start_time outside of the loop and then adding stage_duration to it on every iteration. With the current implementation, stage start times are expected to drift forward from where you would naturally expect them to be by about self.timeout/2 seconds per phase. This probably won't matter at all for most people tbh, but it's also a relatively easy change.
There was a problem hiding this comment.
Good suggestion.
There was a problem hiding this comment.
@LeonChambers ah, I'm remembering why I wrote it this way.
Imagine you have a 19hr stage with a 12hr night and a 12hr day. What should happen?
You could enforce the stage duration and truncate the last phase:
stage |-------------------|
phase |------------|------|
Or you could allow the phase to run its course and overrun the stage time
stage |-------------------|
phase |------------|------------|
I prefer the latter, because you can imagine a situation where a night is cut short, followed by an intense day in the next stage and the plant ends up burned. So overrunning the stage seems like the most benign solution... a sensible default.
Of course, this implementation doesn't solve everything. One could also imagine the following mistake happening:
|day|night|day|night|day|
|day|night|day|night|
But I didn't bother to guard against it. Maybe we should!
Bottom line, I think it makes more sense to let the stage time drift long if it means completing phases correctly, since I think this will be a better default for growth outcomes.
Thoughts?
There was a problem hiding this comment.
Ok, I see your point.
Well, we could just consider a recipe with a 19hr stage with a 12hr night and 12hr day to be an invalid recipe and not worry at all about what happens in that case. However, this means that a typo in recipe could often lead to dead plants, which isn't very nice.
Perhaps a better way to handle this is to re-design the recipe format in such a way that a situation like this couldn't happen at all. For example, instead of specifying that a stage last for a particular length of time, we could have the user specify that the stage last for some number of day-night cycles. To me, this seems slightly easier to reason about when writing recipes and prevents the possibility of mathematical errors resulting in dead plants.
There was a problem hiding this comment.
Yeah, I agree it's easier to reason about programmatically, but I think it's less intuitive when writing recipes.
For example, I know a basil takes about 20 days to grow: 3 days to germinate, 7 days to mature, and 10 more days until harvest. During maturity I know my day cycle can be 14hrs etc, etc. Jotting down that "farmer's knowledge" in a recipe is easy, and intuitive even if it's off by an hour, give or take. But converting that farmer's knowledge to number of cycles via back-of-the-napkin math is kind of a pain.
Conversely, if I'm being exact, I can always take out my pencil, do some long division and make sure everything lines up correctly.
Essentially, for this format I think we should be able to jot down something roughly correct, and have the computer sort out the details. Other formats (like time series ) can be more exacting, but the goal for this one was to have a simple text format that conforms to intuition. That is my take anyway.
There was a problem hiding this comment.
I personally think it's much easier to reason about stages as a number of day-night cycles. And if, as we often claim, plants actually grow faster in our system than with other conventional methods, directly transcribing "farmer's knowledge" isn't particularly useful anyway.
Another way to handle this if we keep the current semantics is to have the system basically "check your math" and round the days field to the nearest value that is a multiple of the length of the day-night cycle.
There was a problem hiding this comment.
Upon reflection, I think I'm ok with having days become day_cycles, and be a multiplier for day + night. So day_cycles: 5 would be valid.
Your argument brings me around: it's hard to reason about solar 24hr days overlayed against artificial day lengths, regardless. Better to reason about phases (you know the plant can handle a 14 hr day in this stage, etc) and how many cycles you want it to go through. If you're cool with that, I'll update the PR to reflect.
| phase_count += 1 | ||
| phase_keys = frozenset(phase.keys()) | ||
| phase_duration = hrs_to_seconds(phase["hours"]) | ||
| end_of_phase = (rospy.get_time() - self.start_time) + phase_duration |
There was a problem hiding this comment.
The above comment also applies here
Instead of phase duration. This is easier to reason about, since cycles may not conform to earth day/night cycles.
|
Addressed all comments in latest commit. Tested on Raspberry Pi. |
|
@LeonChambers do you want to do a quick review on the result? |
|
I'm merging this. |
Also:
var/desirednotdesired/var.Gist: SAMPLE RECIPE
Phased recipes have a format like this (see SAMPLE RECIPE for real world example):
{ "_id": "test_short", "_rev": "1-86cba5eba9c01c5839bc4614dac35bb4", "format": "phased", "version": "1.0", "author": "openag", "recipe_format": "vector 1.0", "optimization": [ "general purpose" ], "recipe_name": "default basil", "date_created": 1481616000, "recipe_id": "472309485702983470592837405", "operations": [ { "cycles": 2, "name": "germination", "day": { "hours": 0.001 }, "night": { "light_illuminance": 0, "water_electrical_conductivity": 0, "water_potential_hydrogen": 6, "hours": 0.001, "air_temperature": 30, "air_humidity": 75, "air_carbon_dioxide": 1000 } }, { "cycles": 2, "name": "early", "day": { "hours": 0.001 }, "night": { "light_illuminance": 1.0, "water_electrical_conductivity": 0, "water_potential_hydrogen": 2, "hours": 0.001, "air_temperature": 40, "air_humidity": 80, "air_carbon_dioxide": 400 } } ], "author_id": "457028937405687305698723059" }Tested on Raspberry Pi.