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

Add methods to get list of names of supported parameters for each solver #18

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

traversaro
Copy link
Contributor

This permits to get all the parameters supported by a solver, prerequisite for #8 .

@S-Dafarra
Copy link
Member

Looks good to me. The only remark, would be not to throw errors in case a parameter is not found, as future versions of the solvers might add parameters that are not yet listed as supported

@traversaro
Copy link
Contributor Author

The only remark, would be not to throw errors in case a parameter is not found, as future versions of the solvers might add parameters that are not yet listed as supported

Sorry, it is not clear to me how this is related to the current PR. Is this suggesting a change here or is a general remark?

@S-Dafarra
Copy link
Member

The only remark, would be not to throw errors in case a parameter is not found, as future versions of the solvers might add parameters that are not yet listed as supported

Sorry, it is not clear to me how this is related to the current PR. Is this suggesting a change here or is a general remark?

Not directly, but since the list of parameters has been specified explicitly, I thought to mention it anyhow. For example, I noticed the pattern in the test

REQUIRE(std::find(parametersNames.begin(), parametersNames.end(), "this_is_not_a_supported_parameter") == parametersNames.end());
and I thought it might be problematic if used also in the actual code. But again, it is not related to the code as is right now

@traversaro
Copy link
Contributor Author

The only remark, would be not to throw errors in case a parameter is not found, as future versions of the solvers might add parameters that are not yet listed as supported

Sorry, it is not clear to me how this is related to the current PR. Is this suggesting a change here or is a general remark?

Not directly, but since the list of parameters has been specified explicitly, I thought to mention it anyhow. For example, I noticed the pattern in the test

REQUIRE(std::find(parametersNames.begin(), parametersNames.end(), "this_is_not_a_supported_parameter") == parametersNames.end());

and I thought it might be problematic if used also in the actual code. But again, it is not related to the code as is right now

Yes, that was just a way to check that the vector did not contained something that was not a parameter. I thought this_is_not_a_supported_parameter was a safe bet for a non-existing parameter name, but if we ever add a solver with a parameter named this_is_not_a_supported_parameter we can change the test.

@traversaro traversaro merged commit c61e8e1 into main Nov 18, 2024
2 of 3 checks passed
@traversaro traversaro deleted the fix8 branch November 18, 2024 10:06
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.

5 participants