RSDK-10771: Constraint violation error too verbose #5069
Conversation
|
Emiyare Ikwut-Ukwa seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
1 similar comment
|
Emiyare Ikwut-Ukwa seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
viambot
added
the
safe to test
motionplan/errors.go
Outdated
| @@ -46,8 +46,13 @@ func newIKConstraintErr(failures map[string]int, constraintFailCnt int) error { | |||
|
|
|||
| // build the error message | |||
| errMsg := errIKConstraint | |||
| cutoffPercent := 10.0 | |||
There was a problem hiding this comment.
Const magic numbers like this should go at the top of the file
viambot
added
safe to test
viambot
added
safe to test
motionplan/errors_test.go
Outdated
| "self-collision constraint: violation between gripper-1:clamp and ur5e-modular:wrist_1_link geometries": 97, | ||
| } | ||
|
|
||
| //nolint:lll |
There was a problem hiding this comment.
we shouldn't ignore the linter unless its actually necessary. In this case we can break the long line up over multiple lines. We allow it to go up to 140 which should be plenty.
Same comment on line 30
There was a problem hiding this comment.
I thought about making this comment but it occurred to me that this is a single error message string longer than 140, so to get rid of the nolint would require just splitting the string and +ing them which feels awkward and annoying just to avoid a nolint.
motionplan/errors_test.go
Outdated
| } | ||
|
|
||
| //nolint:lll | ||
| expectedError := errors.New("all IK solutions failed constraints. Failures: { self-collision constraint: violation between gripper-1:clamp and ur5e-modular:forearm_link geometries: 2.91% }, { self-collision constraint: violation between gripper-1:clamp and ur5e-modular:wrist_1_link geometries: 0.97% }, ") |
There was a problem hiding this comment.
This is less feedback on this particular PR and more good practice going forward: tests with very specific strings or floating point numbers end up being pretty fragile since small changes can cause them to fail. Typically I would try to write tests to be less hardcoded, but in this case what you are testing is there error itself. So I don't think theres any improvements you can make here
viambot
added
safe to test
This PR sets a cutoff of 10%, printing all errors if none exceed cutoff, otherwise only those above the cutoff.