Feature/sensor configs #87
Conversation
There was a problem hiding this comment.
Many thanks for adding this! Few thoughts:
- I'm not sure I love this design, mostly because it requires updating every config for what we want to override (overriding seem like a generally useful feature, if we were to add more params etc in the future).
- Splitting the configs also makes these less clear to navigate I think.
- Would YAML composition not work for this use case (I might be missing something)?
- I understand that specifying all param paths to override may be brittle, but the current version also requires every module to know their override_ns, which is also easy to accidentally break or have several configs/nodes write to.
Overall good to go in if we need something to work quickly 👍
Happy to discuss more, I'm probably missing some of the design choices and constraints 😉
|
@Schmluk sorry, realize that this would probably have been easier to talk over in person (but probably won't happen for a little bit)
This doesn't necessarily require updating every config we want to override; the constraints that I want to satisfy are that (i) the configs remain threadsafe and (ii) it's clear what can and cannot be overriden. There just hasn't been any config that I've adapted to this that has only fields that can be overriden
Agreed, but it seems like the least bad solution. I'm open to better solutions, but the ones I can think of (having the projective integrator attached to each sensor, having sensor-dependent parameters of the projective integrator, etc. in the sensor config, having multiple active window instances operating on a shared volumetric map and object tracks all have pretty big downsides in my opinion)
If you mean overlaying extra params when running a lidar version of Hydra versus a camera version of Hydra, no. I realized I wasn't super clear in my write-up, this is for versions of Hydra that are running with multiple heterogeneous sensors at the same time, which means needing some sort of solution (at runtime) to have multiple sets of parameters for the projective integrator that get applied for a specific sensor when the input is from that sensor.
I wanted to do this originally (use the original namespace of the config), but we don't have that information when the overrides are applied (we lose access to the global namespace after parsing). I could write something in I guess an option that I didn't think about is to have some sort of a: 5.0
b: 4.0
_overrides:
foo: {a: 6.0, b: -1.0}
bar: {b: 2.0}struct ProjectiveConfig : config_utilities::OverrideConfig {
float a;
float b;
};
void declare_config(ProjectiveConfig& config) {
field(config.a, "a");
override_field(config.b, "b");
}
const YAML::Node node = // parsed yaml from above;
const auto parsed = config::fromYaml<ProjectiveConfig>(node);
const ProjectiveConfig base = parsed(); // not override, now a=5, b=4
const ProjectiveConfig foo = parsed("foo"); // overiden by foo, now a=5, b=-1
const ProjectiveConfig bar = parsed("bar"); // overriden by bar, now a=5, b=2but I think this still doesn't fully solve the issues you have with the design, because you still would probably want to pass the correct overriden config by arg in most implementations (instead of having to look up the overriden config for every voxel measurement you're computing). Also I'm lazy and didn't bother having a nested namespace in the example, but you'd get the behavior you're asking for in that regard (if the integrator was at |
Probably best for @Schmluk to review if he has time, but relevant for @yunzc (part of what we talked about yesterday) and @marcusabate (there are also associated khronos changes).
Adds ability to specify config overrides for each sensor (to support heterogeneous sensors). Right now (neglecting the top level namespaces and some other necessary config information) this looks like:
My implementation requires every module that wants to support overrides to split their config, but I think that's probably better than either (i) namespacing every module config by sensor and having to update every config file or (ii) trying to mutate the configs for each module when processing a new sensor and having to worry about threading. Let me know though if it seems like there's a better way to implement this.
For the most part, this also only applies to anything in the active window (my assumption is that once an input packet is integrated into a volumetric representation we don't need to worry about differences between sensors)