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

Expose base classes and hide metaclasses #32

Merged
merged 4 commits into from
Mar 21, 2023
Merged

Conversation

Yoshanuikabundi
Copy link
Contributor

@Yoshanuikabundi Yoshanuikabundi commented Mar 15, 2023

This is a bit of a drive-by but I figured it was easier to write this PR than explain it in an issue :/

Previously the abstract base classes for post-processing layers and GCN stacks were private. I realised this when I started to write up some hints on how to write your own and found that the base classes weren't documented. The registry metaclasses, however, are public and documented. I think this could lead to some major confusion, as the actual functionality you need to implement isn't in the metaclass, its in the base class. I think the metaclasses are really implementation details for the string encoding stuff, and they're the metaclasses of the base classes so inheritors shouldn't need to use them explicitly anyway.

So I've...

Changes made in this Pull Request:

  • Made AtomFeatureMeta, BondFeatureMeta, PostprocessLayerMeta and GCNStackMeta private by prepending them with underscores and removing them from __all__ as appropriate
  • Made BaseGCNStack public by adding it to the gcn module re-exports
  • Made Feature public by adding it to the feature module re-exports
  • Updated the GNNModel docstring to use the base classes instead of the metaclasses.

I think this was just an oversight but if it's intended then no worries!

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@Yoshanuikabundi
Copy link
Contributor Author

Yoshanuikabundi commented Mar 15, 2023

Wow that went badly Nevermind, sorted out my error :)

Copy link
Collaborator

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

Seems like a good idea, thanks!

@Yoshanuikabundi Yoshanuikabundi merged commit e2f2738 into main Mar 21, 2023
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