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

Fix mapped paramteres generation for python #183

Closed

Conversation

Kotochleb
Copy link

@Kotochleb Kotochleb commented Mar 19, 2024

This PR fixes incorrectly created parameters for maps in Python.

With this yaml:

my_node:
  my_params:
    params_to_be_mapped: {
      type: string_array,
    }
    __map_params_to_mapped:
      in_map_param: {
        type: bool,
      }

Current Jinja template will create following code:

# declare any new dynamic parameters
for value_1 in updated_params.params_to_mapped:
  updated_params.my_params.add_entry(value_1)

# ...

for value_1 in updated_params.params_to_mapped:
  param_name = f"{self.prefix_}my_params.{value_1}.in_map_param"

# ...

# declare and set all dynamic parameters
for value_1 in updated_params.params_to_mapped:
  updated_params.my_params.add_entry(value_1)

While the expected output is:

# declare any new dynamic parameters
for value_1 in updated_params.my_params.params_to_mapped:
  updated_params.my_params.add_entry(value_1)

# ...
for value_1 in updated_params.my_params.params_to_mapped:
  param_name = f"{self.prefix_}my_params.{value_1}.in_map_param"

# ...

# declare and set all dynamic parameters
for value_1 in updated_params.my_params.params_to_mapped:
  updated_params.my_params.add_entry(value_1)

Namespace of my_params is not taken into account in for loops of the mapped values.

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly to #185, this also will need to handle the case where {{struct_name}} does not actually exist.

@Kotochleb
Copy link
Author

@sea-bass I saw #185, but it did not solve this issue. I tested it once again after you comment and neither edfeb82 nor current main do not work. That is why I still want to keep this PR open.

As you suggested, I added check if {{struct_name}} is not empty, and I also merged main to make sure everything will still work after adding #185.

@Kotochleb Kotochleb requested a review from sea-bass March 28, 2024 11:03
@@ -0,0 +1,1275 @@
# flake8: noqa
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you delete this autogenerated file from this PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. I don't know how it slipped in there. Thanks for the catch

@pac48
Copy link
Collaborator

pac48 commented Mar 29, 2024

Okay, I think I see the issue here. params_to_be_mapped is referred to but it is not at the top-level scope. Currently, the library assumes that because it may be ambiguous in any other case. For example,

my_node:
  params_to_be_mapped: {
     type: string_array,
   }  
  my_params:
     params_to_be_mapped: {
        type: string_array,
      }
      __map_params_to_mapped:
        in_map_param: {
          type: bool,
          }

this is valid YAML but it is unclear which params_to_be_mapped is referred to. So we need a new syntax to support this. I think it would make sense to use a syntax like this: __map_my_params.params_to_mapped.

The current change in this PR breaks the example file here: https://github.com/PickNikRobotics/generate_parameter_library/blob/main/example_python/generate_parameter_module_example/parameters.yaml

@Kotochleb
Copy link
Author

Kotochleb commented Apr 3, 2024

Thank you for clarification. Since the changes I proposed are breaking, I will simply modify my own code then. Feel free to close this PR without merging.

@pac48
Copy link
Collaborator

pac48 commented Apr 3, 2024

@Kotochleb Do you mind opening an issue for this? That way, this feature can potentially be added when I have some time.

@Kotochleb
Copy link
Author

Since the issue is open now I am closing this PR

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

Successfully merging this pull request may close these issues.

3 participants