Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

🐛 Signature for Trajectory.on_env_feedback() Interface should include the arguments used by the Actor class #373

Closed
jreynolds01 opened this issue Jul 14, 2021 · 1 comment
Assignees
Labels
🐛 bug Something isn't working.

Comments

@jreynolds01
Copy link
Contributor

Description

The Trajectory.on_env_feedback() interface defined here
only includes self as an argument.

However, the Actor() class defined here requires that the on_env_feedback() method include additional arguments:

self.trajectory.on_env_feedback(
                event, state_by_agent, action_by_agent, reward if reward is not None else self.env.metrics
            )

When we define a custom Trajectory, we can overwrite this method (and we do), but the difference in signature produces pylint errors:

W0221:Parameters differ from overridden 'on_env_feedback' method

As far as I can tell, every use of on_env_feedback requires that same signature, so it should be represented in the interface.

Screenshots

To Reproduce

Steps to reproduce the behavior:

  1. Write a custom

Expected Behavior

Environment

  • MARO version (e.g., v0.1.1a1):
  • MARO scenario (CIM, Citi Bike):
  • MARO component (Simulation, RL, Distributed Training):
  • Orchestration platform (GraSS on Azure, AKS on Azure):
  • How you installed MARO (pip, source):
  • OS (Linux, Windows, macOS):
  • Python version (3.6, 3.7):
  • Docker image (e.g., maro2020/maro:latest):
  • CPU/GPU:
  • Any other relevant information:

Additional Context

@jreynolds01 jreynolds01 added the 🐛 bug Something isn't working. label Jul 14, 2021
@ysqyang
Copy link
Contributor

ysqyang commented Jul 15, 2021

That is correct @jreynolds01. Thanks for pointing it out. FYI, Trajectory has been revamped to AbsEnvWrapper in the upcoming version, so things will appear quite different. Stay tuned.

@Jinyu-W Jinyu-W closed this as completed Jul 19, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🐛 bug Something isn't working.
Projects
None yet
Development

No branches or pull requests

3 participants