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.

Sign up for GitHub

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

Jump to bottom

refactors FactorGroup class to tie less-closely to EnumerationFactors #49

Merged
merged 9 commits into from
Aug 10, 2021
Merged

refactors FactorGroup class to tie less-closely to EnumerationFactors #49

merged 9 commits into from
Aug 10, 2021

Conversation

NishanthJKumar
Copy link
Contributor

  • FactorGroup now only required var_group
  • EnumerationFactorGroup and PairwiseEnumeratedFactorGroup are now sub-classes of FactorGroup
  • Refactors sanity_check example to work with new FactorGroup classes

Resolves #43

refactors FactorGroup class to tie less-closely to EnumerationFactors
d963109 
* FactorGroup now only required var_group
* EnumerationFactorGroup and PairwiseEnumeratedFactorGroup are now sub-classes of FactorGroup
* Refactors sanity_check example to work with new FactorGroup classes
@NishanthJKumar NishanthJKumar self-assigned this Aug 9, 2021
StannisZhou
StannisZhou reviewed Aug 9, 2021
View reviewed changes
Copy link
Contributor

@StannisZhou StannisZhou left a comment

Choose a reason for hiding this comment

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

Made some comments. A useful exercise is to also update the heretic model PR with the new updates and see whether things are becoming cleaner/simpler.

pgmax/interface/datatypes.py Outdated Show resolved Hide resolved
pgmax/interface/datatypes.py Show resolved Hide resolved
pgmax/interface/datatypes.py Outdated Show resolved Hide resolved
@NishanthJKumar
Copy link
Contributor Author

I tested the heretic model with this new interface code and it is indeed simpler to specify! Will update the heretic PR once this PR gets merged 😄

StannisZhou
StannisZhou reviewed Aug 10, 2021
View reviewed changes
Copy link
Contributor

@StannisZhou StannisZhou left a comment

Choose a reason for hiding this comment

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

Made some additional comments. Not sure the current approach is a good one....

pgmax/interface/datatypes.py Outdated Show resolved Hide resolved
StannisZhou
StannisZhou reviewed Aug 10, 2021
View reviewed changes
Copy link
Contributor

@StannisZhou StannisZhou left a comment

Choose a reason for hiding this comment

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

Made another round of comments. No major issues, but many details need improvements

pgmax/interface/datatypes.py Outdated Show resolved Hide resolved
pgmax/interface/datatypes.py Outdated Show resolved Hide resolved
pgmax/interface/datatypes.py Outdated Show resolved Hide resolved
pgmax/interface/datatypes.py Outdated Show resolved Hide resolved
pgmax/interface/datatypes.py Outdated Show resolved Hide resolved
pgmax/interface/datatypes.py Outdated Show resolved Hide resolved
pgmax/interface/datatypes.py Outdated Show resolved Hide resolved
pgmax/interface/datatypes.py Outdated Show resolved Hide resolved
StannisZhou
StannisZhou approved these changes Aug 10, 2021
View reviewed changes
Copy link
Contributor

@StannisZhou StannisZhou left a comment

Choose a reason for hiding this comment

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

LGTM after a further simplification!

pgmax/interface/datatypes.py Outdated Show resolved Hide resolved
@NishanthJKumar NishanthJKumar merged commit fe2520b into vicariousinc:master Aug 10, 2021
@NishanthJKumar NishanthJKumar deleted the pairwise_factors branch August 10, 2021 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StannisZhou StannisZhou StannisZhou approved these changes

Assignees

@NishanthJKumar NishanthJKumar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add customized class for pairwise factors; Default to have uniform potentials
2 participants
@NishanthJKumar @StannisZhou