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

refactored product to get_product #190

Merged
merged 3 commits into from
Feb 13, 2024
Merged

refactored product to get_product #190

merged 3 commits into from
Feb 13, 2024

Conversation

IrwinChay
Copy link
Collaborator

@IrwinChay IrwinChay commented Jan 31, 2024

Added get_product()

currently support same scope product, have un-implemented templates for different scope product, the cases written with TODO would need to be discussed (ideally implement them in the actual layers, and keep reparam=self_symb_cfg.reparam in get_product()

The cases without TODO are not urgent and could be ignored for now

Note that to simplify get_product() (avoid too many cases), calling self_layer.get_product() and other_layer.get_product() is different: e.g. if self_layer is input layer and other_layer is product layer, should always call other_layer.get_product(), self_layer.get_product() in this case would raise an error.

Discuss:
**Could you check if id(cls) and id(CLASS_NAME) are different within the class methods? In my case the memory ids are different, so I only used issubclass(other_symb_cfg.layer_cls, cls). Might be a potential bug? **

Discuss how the TODO would be implemented, which are the different cases for different-scope-product we discussed earlier

@IrwinChay IrwinChay requested a review from lkct January 31, 2024 14:22
Copy link
Member

@lkct lkct left a comment

Choose a reason for hiding this comment

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

The following are just notes of the meeting

also more precise docstring for get_product

"""Get the symbolic config to construct the product of this layer with the other layer.

Cases:
- Product with another HadamardLayer: layer config unchanged. \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

docstring: input layer > 2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

List Pre-requisite: expect inputs to match

Copy link
Member

@lkct lkct left a comment

Choose a reason for hiding this comment

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

as below


Alternative usage:
- If performing product between product layer and sum-product layer, break the. \
sum-product layer into sum and product layers beforehand.
Copy link
Member

Choose a reason for hiding this comment

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

automatically do this?

- If performing product between product layer and sum-product layer, break the. \
sum-product layer into sum and product layers beforehand.
- If pushing this layer into the input of the other layer instead, use. \
other_layer.get_product(other_layer, this_layer, push_to_input=True).
Copy link
Member

Choose a reason for hiding this comment

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

not commutative?

Copy link
Member

@lkct lkct left a comment

Choose a reason for hiding this comment

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

only need to pay attention to bold comments. others are minor things that can be postponed.

why no get_product for SumProdL and CP? add # NOTE comment to explain. also explicitly NotImplErr in TuckerL

@IrwinChay IrwinChay force-pushed the symbolic_get_product branch from 2515fef to 45f0895 Compare February 13, 2024 11:29
@lkct lkct merged commit 388fab2 into main Feb 13, 2024
2 checks passed
@lkct lkct deleted the symbolic_get_product branch February 13, 2024 17:26
@lkct lkct added enhancement New feature or request refactor Improve code style and readability and removed enhancement New feature or request labels Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Improve code style and readability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants