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

Fix ExpFamilyLayer #144

Merged
merged 8 commits into from
Nov 8, 2023
Merged

Fix ExpFamilyLayer #144

merged 8 commits into from
Nov 8, 2023

Conversation

lkct
Copy link
Member

@lkct lkct commented Nov 7, 2023 •
edited
Loading

Closes #93
Closes #141

  1. Inspected the bug in Dimension inconsistency in exp family input layer #93. It seems that the bug was actually resolved by Integration and marginalization #121 but we were not sure at that time(?). Anyway the current version should be much clearer and should be correct.
  2. Removed reparam in ExpFamilyLayer and used Reparameterization class instead.
  3. Reworked docstrings for all ExpFamilyLayers and annotated the propagation of tensor shapes.
  4. Removed projection from params to natural params. Natural params are now directly obtained from reparam class.
  5. Unified interface of ExpFamilyLayer to the new Layer interface.

TODO: there should be more unit tests -- tracked by #145

@lkct lkct added documentation Improvements or additions to documentation enhancement New feature or request labels Nov 7, 2023
@lkct lkct requested a review from loreloc November 7, 2023 22:56
@lkct lkct self-assigned this Nov 7, 2023
@lkct lkct mentioned this pull request Nov 7, 2023
Closed
loreloc
loreloc approved these changes Nov 8, 2023
View reviewed changes
cirkit/models/tensorized_circuit.py Show resolved Hide resolved
cirkit/models/tensorized_circuit.py
num_input_units,
num_vars=len(set(v for n in rg_layers[0][1] for v in n.scope)),
num_channels=num_channels,
num_replicas=len(set(n.get_replica_idx() for n in rg_layers[0][1])),
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to add num_replicas() to the region graph.

Copy link
Member

Choose a reason for hiding this comment

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

that is not a property of RGs!

Copy link
Member Author

Choose a reason for hiding this comment

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

will open another PR

Copy link
Member Author

Choose a reason for hiding this comment

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

that is not a property of RGs!

it shouldn't be a property of RG nodes, but that's how we implement now.

cirkit/reparams/exp_family.py Show resolved Hide resolved
@lkct lkct merged commit e013eef into main Nov 8, 2023
2 checks passed
@lkct lkct deleted the fix_exp_family branch November 8, 2023 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arranger1044 arranger1044 arranger1044 left review comments

@loreloc loreloc loreloc approved these changes

Assignees

@lkct lkct

Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use reparam in input layers Dimension inconsistency in exp family input layer
3 participants
@lkct @arranger1044 @loreloc