[WIP] New feature extraction mechanism #72
Conversation
|
I have a first draft of the feature extraction working all the way. You can see the example in My implementation is using a lot of broadcasting. Some stuff is not efficient, like constructing a dictionary all the times. If we only want to keep I decided to use dictionary instead of NamedTuples because it is a bit more flexible in terms of key types. If the ids are integer we cannot do |
| else | ||
| @assert(a.dir == DIR_RIGHT) | ||
| convert(Float64, get(LANEOFFSETRIGHT, scene, roadway, vehicle_index)) | ||
| return lane_offset_right(roadway, scene[vehicle_index]) |
There was a problem hiding this comment.
I think we could be giving a more informative error message here if a.dir has an invalid value, such as
if a.dir == DIR_MIDDLE
...
elseif a.dir == DIR_LEFT
...
elseif a.dir == DIR_RIGHT
...
else
throw(ErrorException("invalid direction"))
end
or similar instead of the assertion (a message like assertion a.dir == DIR_RIGHT failed may be confusing for users).
| # feature(actions, veh) | ||
| # struct TemporalActionFeature <: AbstractFeature end | ||
| # feature(action, veh) | ||
| # struct ActionFeature <: AbstractFeature end |
There was a problem hiding this comment.
Can we already support action features now, out of the box? I think they are expected to occur very soon (in fact, if I want to port my DenseTrafficMerging repo to the new feature extraction interface, I would already need it).
| featuretype(f::Function) | ||
| function used for dispatching feature types depending on the method of the feature function | ||
| """ | ||
| function featuretype(f::Function) |
There was a problem hiding this comment.
If users want to add their own new type of feature, will they have to edit the featuretype function?
There was a problem hiding this comment.
No! :D just implement their function with one of the supported method.
The trait is determined at compile time so there should be no overhead.
There was a problem hiding this comment.
What I meant is a new FeatureType, such as ActionFeature?
There was a problem hiding this comment.
oh yes then, you would need to add a check for the new method.
There was a problem hiding this comment.
My main concern is that it's not easy right now to add actions to the interface (from the user side), so our callbacks effectively don't support actions
There was a problem hiding this comment.
the callbacks do support actions, the feature extraction does not for now.
I think it would require extending the top level function extract_features to have a list of actions as inputs.
There was a problem hiding this comment.
Fair enough we can still use callbacks for collecting action related metrics, that should be good enough - didn't think about that
This PR implements a new feature extraction mechanism and interface.
#70
extract_featuresfunction.DistanceTo(:bob),CollisionWith(:alice), ...). This is conflicting with the use of symbols and needs to be discussed.Most of the features from the previous version are implemented