Skip to content
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

Allow VectorSource to take an N-vector #523

Open
clalancette opened this issue Aug 14, 2018 · 5 comments
Open

Allow VectorSource to take an N-vector #523

clalancette opened this issue Aug 14, 2018 · 5 comments

Comments

@clalancette
Copy link
Contributor

When we added in the VectorSource (#502), it could only output a 1-vector. However, it should be more generic than that and we should be able to make it an N-vector. A relatively straightforward way to do this is:

  1. Make the VectorSource constructor take a map of string -> vector indices that the VectorSource holds onto.
  2. Make the Set() method take a string and a value; it looks up the correct index of the vector to set based on the string, and sets it.
  3. When setting the data into the context, it iterates over all things in the map and sets them all on the output port.
@hidmic
Copy link
Contributor

hidmic commented Aug 15, 2018

@clalancette One question and one thought. Why would you add that map from strings to indices? A VectorSource of N-elements is for sure the way to go for settings that are vectors (e.g. an angular velocity instead of an angular speed), but if one wants N unrelated scalar settings, having N 1-element VectorSources sounds simpler. What do you think?

@clalancette
Copy link
Contributor Author

One question and one thought. Why would you add that map from strings to indices? A VectorSource of N-elements is for sure the way to go for settings that are vectors (e.g. an angular velocity instead of an angular speed),

I think @stonier was thinking to address the scenario where the VectorSource is this (i.e. a vector of related N-elements). With the current VectorSource, we can only do a 1-vector, so this is just an expansion of that. The map of string -> elements was basically to put some easy syntactic sugar on it. Does that make sense?

@hidmic
Copy link
Contributor

hidmic commented Aug 15, 2018

Hmm I'm trying to imagine the use cases. Say your vector setting is a force. Does something like source->Set("x", 12.0) make sense? Now, if your vector were to be a collection of unrelated quantities (e.g. temperature and pressure), I'd advocate for having two separate 1-element sources.

@stonier
Copy link
Collaborator

stonier commented Sep 18, 2018

I had two thoughts here:

Naming

VectorSource miscommunicates the intent when it only ever contributes one element. Either rename it or truly make it capable of being a vector source.

Convenience

If making a system that truly is a vector source, provide the user some syntactic sugar so that any code they write is easily readable, e.g. Set("speed", 12.0) instead of Set(SOME_DYNAMICALLY_INFERRED_INDEX, 12.0) or worse still, the bad habit that drake systems users fall into, Set(1, 12.0).

@stonier stonier added this to the Backlog milestone Sep 18, 2018
@hidmic
Copy link
Contributor

hidmic commented Sep 18, 2018

I agree on naming, though if we extend it I don't see a point in keeping a scalar version.

On convenience, that's exactly the potential use case I had imagined and the one I'm not very fond of. One can use a vector as problem domain entity or as a collection of those. String indexes make a lot of sense when dealing with the latter, e.g. in system states, but are redundant (except as coordinate tags) in the former case. And in that context, I believe system diagrams will be cleaner if we avoid aggregating things like that and rely on having separate ports instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants