additional_data leads to configuration error

[Kazadhum]

Hi @miguelriemoliveira!

As discussed in #1000, I'm implementing continuous data collection for the additional data, just like I did for the regular sensor data. So, I added the following to my config.yaml for testing:

additional_sensor_data:
  imu:
    modality: "imu"
    link: "imu_link"
    parent_link: "flange"
    child_link: "imu_link"
    throttle: 

As a small aside, I got this structure from these lines of code:

atom/atom_calibration/src/atom_calibration/collect/data_collector.py

Lines 295 to 299 in 7122ad1

	if 'additional_data' in self.config:
	for description, value in self.config['additional_data'].items():
	data_dict = {'_name': description, 'modality': value['modality'], 'parent': value['link'],
	'calibration_parent': value['parent_link'], 'calibration_child': value['child_link']}

Upon trying to configure, this error popped up and the configuration failed:

ATOM Error: Config file does not have the correct keys.
Keys that should not exist: ['additional_sensor_data']
Keys that are missing : []

I traced the origin of this error back to this:

atom/atom_core/src/atom_core/config_io.py

Lines 40 to 46 in 7122ad1

	def verifyConfig(config, template_config):
	# Check if all high level keys exist
	same_keys, extra_keys, missing_keys = dictionaries_have_same_keys(config, template_config)
	if not same_keys:
	atomError('Config file does not have the correct keys.\nKeys that should not exist: ' +
	str(extra_keys) + '\nKeys that are missing : ' + str(missing_keys))

And so, it seems like the template for configuration file does not have the additional_data field.

I opened this issue to discuss whether or not it is worth it to include it again. It should be relatively easy to do, but I ask because maybe it was deleted from the template for a reason, and ultimately would only be useful for when we want to capture data from a sensor the pose of which we do not want to calibrate. However, at least the data collector script seems ready to receive additional_data in the configuration. To maintain backwards compatibility, I would have to add an exception to that error in the verifyConfig() function, like I did for the continuous sub-field.

What do you think I should do?

[miguelriemoliveira]

Hey @Kazadhum ,

I would say that if we deprecated it then perhaps its better not to include it again.
Moreover, you don't actually need it do you?

[miguelriemoliveira]

I would propose that you do a new field called something like continuous data or something,

How do you propose to store the (continuous) imu messages?

[Kazadhum]

Yes, I don't need it, so I think I agree with you that it's not worth including again.

I would propose that you do a new field called something like continuous data or something,
How do you propose to store the (continuous) imu messages?

This is exactly what I have implemented at the moment.
Here is a print from a dataset

So I have a top-level continuous_sensor_data field, with a sub-field for each sensor.

[miguelriemoliveira]

Sounds good. But the imu should be the sensor name and as such should be an instance like name, e.g. arm imu or imu 1.
The term "imu" alone is the modality,

One other thing. How will you have the collection stamp? From the data in the collections, i.e., transforms, or sensor data ...

[Kazadhum]

Sounds good. But the imu should be the sensor name and as such should be an instance like name, e.g. arm imu or imu 1. The term "imu" alone is the modality,

You're right, changing this in the configuration...

One other thing. How will you have the collection stamp? From the data in the collections, i.e., transforms, or sensor data ...

Currently, it looks like this:

I believe this is the same organization as in the collections for other sensors, with the sensor data at the same level as the header.

[miguelriemoliveira]

Yes, that's it. Thanks.

[Kazadhum]

Thank you! Closing this issue now...