SDFRandomizer force poisitive location

[HorvathDawson]

Is there any reason that the force positive for the SDF randomize happens on the sample instead of the result from each method??

In my eyes the force positive should act right before the value is set to the new value otherwise you will only have positive variations to the default value with additive.

gym-ignition/python/gym_ignition/randomizers/model/sdf.py

Lines 294 to 313 in 311b08e

	if data.force_positive:
	sample = max(sample, 0.0)
	# Update the value
	if data.method is Method.Absolute:
	data.element.text = str(sample)
	elif data.method is Method.Additive:
	default_value = self._default_values[data.element]
	data.element.text = str(sample + default_value)
	elif data.method is Method.Coefficient:
	default_value = self._default_values[data.element]
	data.element.text = str(sample * default_value)
	else:
	raise ValueError("Method not recognized")

[diegoferigo]

This seems like a bug, thanks! It makes sense to enforce positiveness after applying the randomization function. Since I'm about to make a new release that supports the new Fortress distribution, I'll fix it in devel.

[diegoferigo]

Should be fixed after #400.

[diegoferigo]

@HorvathDawson since you're using this class, I want to bring to your attention a limitation I found a while ago #218, be aware of that.

[HorvathDawson]

@diegoferigo I actually already found that bug on my own before seeing #218. I saw you have a recommend future implementation.

I didn't understand exactly how it would be implement since when you pass an XPath for inertia it will pass all inertia for every link. So without putting a bunch of logic into the sdf randomizer class it would be tricky unless I missed something obvious.

Another feature that would be nice in the sdf randomizer is another chained method which is "if not found create". Instead of having to do this manually in the randomizer class.

I tried installing gym-ignition from source to work on the implementation but I had a ton of issues from idyntree on my installation I didn't feel like debugging.

[diegoferigo]

@diegoferigo I actually already found that bug on my own before seeing #218. I saw you have a recommend future implementation.

I didn't understand exactly how it would be implement since when you pass an XPath for inertia it will pass all inertia for every link. So without putting a bunch of logic into the sdf randomizer class it would be tricky unless I missed something obvious.

The idea was to collapse all the elements inside //link/inertial/inertia in a single randomization entry and randomize the entire inertia matrix instead of the individual parameters. However, this approach would trade off with the granularity of the selected randomization method, since the simplest implementation would apply the same distribution configuration to all the matrix elements. In any case, the issue is still open because the logic would be indeed not straightforward, it's not something that has a top priority from our side right now.

Another feature that would be nice in the sdf randomizer is another chained method which is "if not found create". Instead of having to do this manually in the randomizer class.

I'm not fully convinced about this one, doing it automatically would mean introducing the definition of exclusions and the logic could be too convoluted. I'd tend towards keeping things more declarative to follow the principle of least astonishment. In most cases, users have control over the models and can add all the parameters they need beforehand, doing it from the code is just a workaround I used because I didn't want to vendor models.