feat: Generalize edge type polymorphism. #170
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
norhan-synnada
requested review from
kberat-synnada,
mehmetozsoy-synnada and
aturker-synnada
github-actions
bot
added
physical
logical
tests
common
models
constraints
utils
framework-utils
codegen
labels
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #170 +/- ##
==========================================
+ Coverage 88.79% 88.81% +0.01%
==========================================
Files 61 61
Lines 15931 15964 +33
==========================================
+ Hits 14146 14178 +32
- Misses 1785 1786 +1
|
norhan-synnada
changed the title
mithril/framework/common.py
Outdated
| # ) | ||
| def __post_init__(self) -> None: | ||
| # Convert to generic Tensor type if Tensor type is provided. | ||
| if self.type is Tensor: |
There was a problem hiding this comment.
could we remove post_init and handle Tensor in set_type
| @@ -192,29 +189,36 @@ def edge_type_constraint( | |||
| ) -> ConstrainResultType: | |||
| updates = Updates() | |||
| status = False | |||
| tensor_exists: bool = False | |||
| tensor_output: bool = output.is_tensor | |||
| for input in inputs: | |||
There was a problem hiding this comment.
Small thing, but I think we can do tensor_exists check similar to status check in line 221.
tensor_exists = any(input.is_tensor for input in inputs)| # If no ToBeDetermined edge_type exists, return True. | ||
| if ToBeDetermined not in input_edge_types: | ||
| # If no polymorphic edge_type exists, return True. | ||
| if all({not input.is_polymorphic for input in inputs}): |
There was a problem hiding this comment.
-
allandanysupports comprehension directly. So we do not need to put it in asetfirst -
Since we are setting status in there already, I think there is no need to setting status occasionally inside the function. It seems to me it would be correct if we return as follows:
return all(not input.is_polymorphic for input in inputs), updates| # if physical_data.edge_type not in (Tensor, ToBeDetermined): | ||
| if not ( | ||
| physical_data.is_tensor | ||
| or physical_data.edge_type is ToBeDetermined |
There was a problem hiding this comment.
could this check be as follows?
if not physically_data.is_tensor or phsical_data.is_polymorphic| for s_types in (subtypes_1, subtypes_2): | ||
| other_set = subtypes_2 if s_types == subtypes_1 else subtypes_1 | ||
| for orig_type in (list, tuple, range): | ||
| if orig_type in s_types: |
There was a problem hiding this comment.
Not critical but, instead of using
if orig_type in s_types: if orig_type not in s_types:
continuecould be used, which will look better.
aturker-synnada
approved these changes
Jan 30, 2025
kberat-synnada
approved these changes
Jan 30, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Closes #168
User can define polymorphic type declarations other than ToBeDetermined like Tensor[int | float] | int | float.
What is Changed
Include the changes introduced in this PR.
Checklist: