Check for type in set_parameter_value of parameter maps

[SprGrf]

The following does not inherently check for type:

this->parameter_map_.set_parameter_value(parameter_name, parameter_value);

control-libraries/source/state_representation/include/state_representation/parameters/ParameterMap.hpp

Line 91 in 3922662

void set_parameter_value(const std::string& name, const T& value);

Use of this function does not check for type, only directly inserts or assigns the value. To be fair, it is mentioned right below that the validation part should be overwritten:

control-libraries/source/state_representation/include/state_representation/parameters/ParameterMap.hpp

Line 107 in 3922662

virtual void validate_and_set_parameter(const std::shared_ptr<ParameterInterface>& parameter);

But probably it could be made more clear. The current workaround is to get the parameter interface and then assign the value:

this->parameter_map_.get_parameter(parameter_name)->set_parameter_value<T>(parameter_value);

[domire8]

Yeah hard to say what to do here. I think the easiest - yet breaking - solution would be to remove set_parameter_value entirley. If we keep it in then I'd be in favor of check if the parameter already exists and if so, set it's value from the provided argument. This might then raise an error depending on compatibility.

As it stands, set_parameter_value is misleading because it doesn't actually try to set the value of the parameter but it constructs a new parameter object and stores it.

@eeberhard

[eeberhard]

hmm I think the intention was not to make any assumptions about the usage of the ParameterMap and allow the validation of name and type to be decided later, defaulting to insert_or_assign behavior. This means it just holds a collection of parameters, but those parameters can indeed be overwritten at any time.

Also I suppose I might have considered set_parameter_value to mean "Set a parameter by value" since all it does is make_shared_parameter<T>(name, value) as a convenience.

My question is, do you want to keep set_parameter unchanged (making new parameters and overwriting if they already exist) or do your comments also apply there?

[domire8]

I would leave set_parameter unchanged because as you say we wanted

not to make any assumptions about the usage of the ParameterMap and allow the validation of name and type to be decided later

[eeberhard]

I would leave set_parameter unchanged

I actually disagree in this instance, and think that set_parameter and set_parameter_value should either both be permissive (with insert_or_assign) OR both potentially throw (because a parameter was already set with a different type). The middle case where set_parameter is permissive but set_parameter_value is not is a bit weird IMO.

It seems mostly a question of expectation of what set_parameter_value should do. If it helps, we could mark set_parameter_value as deprecated and introduce an overloaded method set_parameter(name, value) instead (e.g. to set parameter by value). Actually deleting set_parameter_value could then wait for a breaking version change.

The default behavior of the core class should not necessarily change but a StrictParameterMap that derives and overrides the validate_and_set_parameter could check if the parameter already exists in the map. If not, it creates a new one and inserts it. If yes, it would try to update the existing value iff the types match, else throw an exception.

[domire8]

If it helps, we could mark set_parameter_value as deprecated and introduce an overloaded method set_parameter(name, value) instead (e.g. to set parameter by value). Actually deleting set_parameter_value could then wait for a breaking version change.

I would be in favor of that yes.

StrictParameterMap that derives and overrides the validate_and_set_parameter could check if the parameter already exists in the map.

That would also be a great optional addition yes.

[domire8]

If it helps, we could mark set_parameter_value as deprecated and introduce an overloaded method set_parameter(name, value) instead (e.g. to set parameter by value). Actually deleting set_parameter_value could then wait for a breaking version change.

Note that this change will propagate to controllers and dynamical systems as they both inherit from the ParameterMap.