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

Investigate adding acquisition metadata #766

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

uri-granta
Copy link
Collaborator

@uri-granta uri-granta commented Jul 11, 2023

Related issue(s)/PRs:

Summary

Investigate adding acquisition metadata to rules, acquisition function builders and the ask tell interface (and possibly elsewhere). This can be used to make acquisition function updates depend on more than just the models and data.

There are at least four approaches here:

  1. Breaking API change to add metadata everywhere. Pros: simple to define new builders. Cons: breaking API change affecting every builder and rule, version bump to 2.0.
  2. Non-breaking change extending existing builder and rule classes. Pros: doesn't break existing code, uses existing classes. Cons: defining metadata-using classes is ugly as it requires overriding two methods not one.
  3. Non-breaking change introducing new builder class. Pros: doesn't break existing code, simple to define new builders. Cons: introduces new classes for this use case (and may require more classes in future such as MetadataGreedyAcquisitionFunctionBuilder)
  4. Do nothing! Pros: no breaking changes, uses existing infrastructure, no unstructured data. Cons: less flexible (though it would be good to find an actual use case that illustrates this).

Since we don't currently have a convincing use case, my vote goes to 4. If we find one then we could consider 1, while perhaps holding off breaking the API until we're ready to do a 2.0 release for other reasons.

Fully backwards compatible: depends

PR checklist

  • The quality checks are all passing
  • The bug case / new feature is covered by tests
  • Any new features are well-documented (in docstrings or notebooks)

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.

1 participant