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

[type] [refactor] Decouple quant from SNode 2/n: Remove physical_type from QuantIntType #5223

Merged

Conversation

strongoier
Copy link
Contributor

Related issue = #4857

Having a physical_type in QuantIntType is simply wrong - a QuantIntType may be placed under different bit_structs. The physical_type needs to be manually passed into functions.

@netlify
Copy link

netlify bot commented Jun 21, 2022

Deploy Preview for docsite-preview canceled.

Name Link
🔨 Latest commit 836cbe4
🔍 Latest deploy log https://app.netlify.com/sites/docsite-preview/deploys/62b1b21085fcf7000812a84e

@k-ye
Copy link
Member

k-ye commented Jun 21, 2022

a QuantIntType may be placed under different bit_structs.

Out of curiosity, does it make sense if we have a MaterializedQuantInt (or PlacedQuantInt) or something as an abstraction in the future? It basicallys contains a QuantIntType + a physical storage type.

@strongoier
Copy link
Contributor Author

Out of curiosity, does it make sense if we have a MaterializedQuantInt (or PlacedQuantInt) or something as an abstraction in the future? It basicallys contains a QuantIntType + a physical storage type.

Yep. In fact my original thought is to create a BitStructMemberType which contains a BitStructType + a member_id for those placed types. However, I haven't figured out a perfect way to make it also serve as a QuantIntType yet. Do you have any suggestions here? Is using inheritance feasible (it seems that we don't use inheritance in Taichi types though..)?

@k-ye
Copy link
Member

k-ye commented Jun 22, 2022

Do you have any suggestions here? Is using inheritance feasible (it seems that we don't use inheritance in Taichi types though..)?

I can't say that I have enough context here LOL... My current understanding is that, QuantIntType (QIT) is a computational type without the knowledge of the storage type. So by combining QIT with a storage type, you have fully materialized info. (I'm not sure if it's still appropriate to call this materialized info a "type")

But anyhow, I think the cleanup in the current PR looks good!

@strongoier
Copy link
Contributor Author

I can't say that I have enough context here LOL... My current understanding is that, QuantIntType (QIT) is a computational type without the knowledge of the storage type. So by combining QIT with a storage type, you have fully materialized info. (I'm not sure if it's still appropriate to call this materialized info a "type")

But anyhow, I think the cleanup in the current PR looks good!

Your understanding is correct. The only issue is that the storage type is a bit complicated, especially when it comes to floating points. I'll tweak the implementation a bit more to see what is needed in the end.

BTW give me a LGTM plz :-)

Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

LGTM!

@strongoier strongoier merged commit 05d2d61 into taichi-dev:master Jun 22, 2022
@strongoier strongoier deleted the remove-physical-type-from-quant-int branch June 22, 2022 07:57
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