-
Notifications
You must be signed in to change notification settings - Fork 190
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 low dimension interest observation. #1977
Conversation
We also need episode termination or done criteria based on termination of vehicle of interest. |
I am in the process of adding it. |
smarts/core/agent_interface.py
Outdated
class ActorsAliveDoneCriteria: | ||
"""Require actors to persist.""" | ||
|
||
actors_of_interest: Tuple[str, ...] = () | ||
actors_filter: Tuple[str, ...] = () | ||
"""Actors that should exist to continue this agent.""" | ||
|
||
strict: bool = True | ||
"""If strict the agent will be done instantly if a target actor is not available | ||
immediately. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove ActorsAliveDoneCriteria
, because we already have (i) ScenarioInterestDoneCriteria
and (ii) AgentsAliveDoneCriteria
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have slightly different uses. One is specified at the scenario generation level, the other solely in the interface. Maybe I could merge the two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, do consider merging the underlying code for both of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged the two. Now agent interface defined actors of interest also show up in the observations.
@cached_property | ||
def actors_pattern(self) -> re.Pattern: | ||
"""The expression match pattern for actors covered by this interface specifically.""" | ||
return re.compile("|".join(rf"(?:{aoi})" for aoi in self.actors_filter)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to place this method elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Placing it elsewhere would complicate its use. It is fine since it has no side effects.
My consideration is if I should compress actors_pattern
and actors_filter
to be a single attribute since it is not clear that actors_filter
uses regular expression matching.
Co-authored-by: adai <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have merged this branch on my branch and tested with several scenarios, the episodes now end with the vehicle of interest done, thanks Tucker!
oops, wrong PR, was intented to comment under #1995 |
Closes #1918