Skip to content
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

High-level API: Establish a strong link between the actor and the distribution function #1195

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

opcode81
Copy link
Collaborator

@opcode81 opcode81 commented Aug 7, 2024

Establish a strong link between the actor and the distribution function (dist_fn) used in policies by creating the distribution function in the actor factory which knows which function is appropriate.

Consequently, remove the policy parameter 'dist_fn' from the high-level API because it is determined automatically, eliminating the possibility of misspecification by the user. [breaking change: code must not specify the 'dist_fn' parameter, but persisted objects continue to work as expected]

Implements #1194

  • I have added the correct label(s) to this Pull Request or linked the relevant issue(s)
  • I have provided a description of the changes in this Pull Request
  • I have added documentation for my changes and have listed relevant changes in CHANGELOG.md
  • If applicable, I have added tests to cover my changes.
  • I have reformatted the code using poe format
  • I have checked style and types with poe lint and poe type-check
  • (Optional) I ran tests locally with poe test
    (or a subset of them with poe test-reduced) ,and they pass
  • (Optional) I have tested that documentation builds correctly with poe doc-build

@opcode81 opcode81 added enhancement Feature that is not a new algorithm or an algorithm enhancement refactoring No change to functionality high-level-api Mainly affects tianshou.highlevel labels Aug 7, 2024
@opcode81 opcode81 force-pushed the feature/actor-dist-fn-link branch 2 times, most recently from d97f7d3 to ae726fd Compare August 7, 2024 13:46
@opcode81 opcode81 changed the title High-level API: Establish a strong link between the actor and the distribution link High-level API: Establish a strong link between the actor and the distribution function Aug 7, 2024
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 89.18919% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.77%. Comparing base (fb0561a) to head (8114dd1).
Report is 1 commits behind head on master.

Files Patch % Lines
tianshou/highlevel/params/dist_fn.py 81.81% 2 Missing ⚠️
tianshou/highlevel/module/actor.py 95.00% 1 Missing ⚠️
tianshou/highlevel/params/policy_params.py 66.66% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1195      +/-   ##
==========================================
+ Coverage   84.74%   84.77%   +0.02%     
==========================================
  Files         104      104              
  Lines        8974     8983       +9     
==========================================
+ Hits         7605     7615      +10     
+ Misses       1369     1368       -1     
Flag Coverage Δ
unittests 84.77% <89.18%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@MischaPanch MischaPanch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, only minor comments.

Unrelated comment: At some point soon we should add more exhaustive documentation of the ideas behind HL interfaces - I have a hard time understanding why AgentFuture would be needed from a glance at it (I could search in the IDE, ofc, and would eventually understand it, but we should try to reduce the cognitive load)

tianshou/highlevel/module/actor.py Show resolved Hide resolved
tianshou/highlevel/module/actor.py Show resolved Hide resolved
tianshou/highlevel/params/dist_fn.py Outdated Show resolved Hide resolved
@opcode81 opcode81 force-pushed the feature/actor-dist-fn-link branch from ae726fd to c81a386 Compare August 7, 2024 22:36
@opcode81
Copy link
Collaborator Author

opcode81 commented Aug 7, 2024

Unrelated comment: At some point soon we should add more exhaustive documentation of the ideas behind HL interfaces - I have a hard time understanding why AgentFuture would be needed from a glance at it (I could search in the IDE, ofc, and would eventually understand it, but we should try to reduce the cognitive load)

Sure, we can add more information where it helps.

My general documentation principle is to document what things are, not how they are used, because the former is invariant/universal while the latter is just a momentary snapshop (and could potentially change in the future). It's usually fine (for me) to search for usages to find out how something is being used.
Also, my principle is to ignore stuff that doesn't momentarily concern me and assume that it's fine. You shouldn't care what ActorFuture is for if you aren't changing a piece of code that uses it.

To reveal: ActorFuture is necessary to support the concept of critics reusing the actor network. When the user declares that he wants to reuse the actor for the critic, I have to create a critic factory that does this, but the actor does not exist yet. So the critic factory instead receives the future, which will eventually be filled when the actor factory is called. When the critic factory is itself called, it can use the then-filled actor to create the critic.

distribution function (dist_fn) used in policies by creating
the distribution function in the actor factory which knows which
function is appropriate.

Consequently, remove the policy parameter 'dist_fn' from the high-level API
because it is determined automatically, eliminating the possibility
of misspecification by the user. [breaking change: code must not
specify the 'dist_fn' parameter, but persisted objects continue to work
as expected]

Implements #1194
@opcode81 opcode81 force-pushed the feature/actor-dist-fn-link branch from c81a386 to ee90cb5 Compare August 7, 2024 23:11
@MischaPanch MischaPanch marked this pull request as ready for review August 8, 2024 06:37
@opcode81 opcode81 force-pushed the feature/actor-dist-fn-link branch from 7f1e1a8 to 8114dd1 Compare August 8, 2024 08:08
@maxhuettenrauch
Copy link
Collaborator

Would it make sense to leave the user the option of providing a custom dist_fn optionally? Then of course they need to know what they are doing. I can't think of a practical example right now, but who knows?

@opcode81
Copy link
Collaborator Author

opcode81 commented Aug 8, 2024

Would it make sense to leave the user the option of providing a custom dist_fn optionally? Then of course they need to know what they are doing. I can't think of a practical example right now, but who knows?

@maxhuettenrauch it is still possible to modify the distribution function, simply by using your own (subclassed) ActorFactory which can override the creation method to return whatever you want. So only the mechanism changed...

@MischaPanch MischaPanch merged commit bd74273 into master Aug 8, 2024
4 checks passed
@MischaPanch MischaPanch deleted the feature/actor-dist-fn-link branch August 8, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature that is not a new algorithm or an algorithm enhancement high-level-api Mainly affects tianshou.highlevel refactoring No change to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants