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

Add autora-theorist-toolkit as an optional dependency of autora #521

Closed
wants to merge 3 commits into from

Conversation

TheLemonPig
Copy link
Collaborator

Description

Initial PR to pull the toolkit into AutoRA as a pre-release

Type of change

  • feat: New AutoRA subpackage
  • refactor: Reimplemented BMS included
  • docs: Toolkit documentation stand-in

Features (Optional)

  • AutoRA subpackage; stand-in documentation; theorist toolkit for implementing symbolic regression theorists; bms implementation

Remarks (Optional)

  • Expect there may be a fair amount of outstanding work needed before it meets AutoRA standards for documentation and tests.

@TheLemonPig
Copy link
Collaborator Author

I followed the documentation instructions for adding a package to AutoRA, which meant making the repo locally. Would it make sense to make it an AER repository?

Copy link
Collaborator

@musslick musslick left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution. I'm just not sure what this toolkit is meant for?

Is it a summary package for all theorists? If so, then BMS, BSR, and DARTS should be a part of it.

Is it subpackage for a theorist? If so, then what does it offer beyond the theorist cookiecutter template? There appears to be also a doc missing, so it's unclear what it does...

@hollandjg
Copy link
Member

Hi Joshua! I see what you've done – created a package with common utilities for dealing with general computation trees, and applied that to the SR, BSR and BMS approaches. I like it!

Before we merge it, I think you'll need to think about how best to use it. The current approach for AutoRA is that each model has its own repository, and BMS/BSR follow that. What you could do is make the current theorist-toolkit with the models into a more specific computation-tree-library without the models, and then import that as a dependency in each of the BSR/BMS packages. That would mean that you wouldn't break any existing functionality, and people who were using BMS/BSR get the benefit of the updated backend.

But before you make a package like that which serves as the foundation of other packages, you'll need a couple of other bits. Things like:

  • Check whether there's already another package we could use to get the same functionality. If you publish a package like this and make it the foundation of a bunch of other packages, you'll have to support it long-term. If you're able to use a symbolic package like sympy to do the same thing or to get a long way there, that might reduce your maintenance load and give you additional functionality for free, and allow us to close some existing issues (Use the same operation names for DARTS, BMS, and BSR  #301).
  • Present it and discuss it: bring it to the group, get feedback on how it works and looks and get some reviewers who know BSR and BMS who can help you with PRs on the package itself.
  • Test it extensively: make sure each class and function in the package does what it's supposed to do in the normal case, in edge cases, and in outside cases (when it fails, how should it fail?)
  • Decide what its "thing" is: Is it just relevant to theorists or is it more general? (I think it's more general.)
  • Think about the structure: Do you want people to just import all of the functions and classes from a single place, like from autora.utils.computation_tree import Tree, replace_node, Temperature or from subpackages?

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.

3 participants