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

Fixing callbacks PR comments and other misc improvements. #350

Merged
merged 18 commits into from
Aug 16, 2022

Conversation

Lucaweihs
Copy link
Collaborator

PR:

  1. Fixes the PR comments corresponding to PR Add Callback Support #339. In particular, callbacks that want data from tasks need to return a list of sensors that will be called on the task.
  2. Introduces a number of other small misc improvement (a new CPCA loss ViT preprocessor, and better error handling).

Copy link
Collaborator

@jordis-ai2 jordis-ai2 left a comment

Choose a reason for hiding this comment

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

It looks good to me. I especially appreciate the effort in self-documenting the callbacks. Great job!

allenact/base_abstractions/callbacks.py Show resolved Hide resolved
######## CPCA Softmax variants ######


class CPCA1SoftMaxLoss(CPCASoftMaxLoss):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit tedious to have so many similar classes just because of the UUID. Would it be possible to have the uuid be an argument to __init__ and have some sort of factory that gives us the right instance given the needed uuid? Didn't think thoroughly about the cons of such a design, so feel free to ignore if it's completely out of scope.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting! I could even imagine that the uuid could be something like "CPCASoftMaxLoss(planning_steps=8)" and then we could do some reflection to instantiate the class. For now I'll merge but will keep this in mind for the future.

@Lucaweihs Lucaweihs merged commit e89eae1 into callbacks Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants