rework groundtruth handling #150
Conversation
floweisshardt
changed the title
atf_core/src/atf_core/atf.py
Outdated
| rospy.loginfo("no user result set for testblock \'%s\'"%testblock) | ||
| rospy.logwarn("no user result set for testblock \'%s\'"%testblock) | ||
| metric_result = MetricResult() | ||
| metric_result.groundtruth.error_message = "!!USER ERROR!!: no user result set for testblock %s"%testblock # TODO use from global field (same as in atf.stop()) |
There was a problem hiding this comment.
actually, I think we do not need to set the message via a global field
because if you set the result to false, then this us a valid MetricResult which will show up as properfailure during analysis phase...
|
|
||
| metric_result.data = self.metric_result.data | ||
| # check if result is available | ||
| if metric_result.groundtruth.error_message.startswith("!!USER ERROR!!: no user result set"): # TODO use from global field (same as in atf.stop()) |
There was a problem hiding this comment.
I dont think we need this block because in this case, metric_result is already sufficiently filled (see my comment above) and will correctly be handled as any other case where result is set to false...right?
There was a problem hiding this comment.
I'm still in favor of keeping this block as it gives a different failure reason. I calling atf.stop() without user result we se the following failure reasons:
without this block
Failed ATF tests:
- test 'ts0_c0_r0_e0_s0_0' (test1, robot1, env1, testblockset1):
- testblock 'testblock_5s':
- metric 'user_result::0': groundtruth missmatch: 0.000000 not within 0.800000+-0.200000
with this block
Failed ATF tests:
- test 'ts0_c0_r0_e0_s0_0' (test1, robot1, env1, testblockset1):
- testblock 'testblock_5s':
- metric 'user_result::0': testblock testblock_5s stopped without user_result
There was a problem hiding this comment.
I'm asking myself now why result and error_message are part of the Groundtruth.msg and not directly under MetricResult.msg
with your explanations from yesterday, I now understand groundtruth is rather optional and contains only the "target" value and the epsilon range
having result and error_message in Groundtruth.msg doesn't seem to make too much sense
I rather think those two fields should be put (on level up) into MetricResult directly
as result and error_message are not necessarily groundtruth-related but are mandatory (at least result) also for metrics that do NOT use groundtruth
if you then would chose uint8 (together with some enums) for result instead of bool - or even actionlib_msgs.GoalStatus directly, then the default value means something PENDING/UNINITIALIZED/NOT_SET and would make it easier to distinguish result not being set from result being negative/failure or positive/success
so any metric would not try to set the value of metric_result.groundtruth.result but rather metric_result.result - or whatever the field is called then
we can continue yesterday's discussion in another daily if you like
There was a problem hiding this comment.
this is just a scroll-through review....most comment apply to several files and multiple appearances within each file but are only commented once...
in particular you might want to find all occurrences of:
active,startedandfinished :point_right: should be removed and replaced bystatus` enumgroundtruth_resultorgroundtruth.resultand make sure they are not involved in boolean operations (i.e.True/Falseassign or boolean operator without==/!=) and replace them accordingly to the status Groundtruth result enum
There was a problem hiding this comment.
you also might want to have a branch that introduces pylint checks!
create it on top of this branch
even if it doesn't pass, it's nice to get some hints and possibly fix some easy issues
until full pylint compatibility is achieved that branch can be rebased continuosuly...
|
FYI: pylint is activated in #152 |
those are unsupported metrics. I removed them in #155 and created a backup branch https://github.com/floweisshardt/atf/tree/feature/old_metrics if we want to reanimate them sometime. |
|
@fmessmer I'd suggest to merge this PR and not wait for the pylint fixes in #153. I'll add temporary commits to use this feature branch in
|
|
if the metrics are removed in #155, this PR should get rebased on top of |
floweisshardt
force-pushed
the
fix/user_result
branch
from
24ba26a to
6a02908
Compare
|
PR is now rebased on floweisshardt/master |
No description provided.