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

Fix Sup3rGan interface #118

Merged
merged 4 commits into from
Nov 19, 2022
Merged

Fix Sup3rGan interface #118

merged 4 commits into from
Nov 19, 2022

Conversation

malihass
Copy link
Collaborator

@malihass malihass commented Nov 18, 2022

Adding additional abstract class that contains training functions that can be used by other types of models such as conditional moment models.
Remove Sup3rGen which was only useful in the context of conditional moment learning. This class will be replaced by Sup3rCondMom when conditional moments models are ready to be uploaded.

@grantbuster
Copy link
Member

@malihass I like this a lot better, what do you think? Looks like it turned out to be mostly renaming/organizing but i think it's a more logical organization.

One comment: Would you want to remove meta, training_features, and output_features from AbstractSingleModel? These are in AbstractInterface and I don't see a case where you'd use the SingleModel without the Interface. You could also do AbstractSingleModel as a subclass but i kind of like them separate, seems more modular.

@malihass
Copy link
Collaborator Author

Ah good catch, I forgot to remove them. I had to add them because I had other old methods that I had included in the SingleModel one before and eventually decided against it.
I prefer to have the two abstract classes separate for now in case we need these methods separately.

@malihass malihass merged commit 2e084b7 into main Nov 19, 2022
@malihass malihass deleted the interfaceGan2 branch November 19, 2022 00:02
github-actions bot pushed a commit that referenced this pull request Nov 19, 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.

None yet

2 participants