dsl: Add functionality to intersect SubDimensions #2222
Conversation
…tion of multiple Subdimensions
EdCaunt
requested review from
FabioLuporini,
mloubout,
rhodrin and
georgebisbas
Codecov Report
@@ Coverage Diff @@
## master #2222 +/- ##
==========================================
- Coverage 87.11% 87.05% -0.06%
==========================================
Files 228 228
Lines 40810 40840 +30
Branches 7471 7486 +15
==========================================
+ Hits 35551 35553 +2
- Misses 4653 4681 +28
Partials 606 606
|
| @@ -607,6 +607,65 @@ def middle(cls, name, parent, thickness_left, thickness_right, local=False): | |||
| thickness=((lst, thickness_left), (rst, thickness_right)), | |||
| local=local) | |||
|
|
|||
| @classmethod | |||
| def intersection(cls, sdims): | |||
There was a problem hiding this comment.
I think this should not be a class method, similar to Set that defines intersection as set().intersectio not Set.intersection.
There was a problem hiding this comment.
I think it wants to be a class method since you could conceivably want the intersection of several SubDimensions, so it would want to be intersect(subdimensions) rather than intersect(self, other) imo
There was a problem hiding this comment.
You can still have multiple ones, again similar to a set set(a).intersection(a,b,c,d,e,f) there is nothing that prevents it. to have more than one without being a class method
There was a problem hiding this comment.
True, but then you have to arbitrarily pick one to be the "important" one with which you are intersecting all the others
| def intersection(cls, sdims): | ||
| # Check all SubDimensions supplied overlap | ||
| # NOTE: Think the test for overlap isn't strict enough? | ||
| if not all([sdims[0].overlap(sdim) for sdim in sdims[1:]]): |
There was a problem hiding this comment.
Would make it easier here to with just self.overlap ...
| if not all([sdims[0].overlap(sdim) for sdim in sdims[1:]]): | ||
| raise ValueError("SubDimensions do not all overlap") | ||
|
|
||
| name = '_n_'.join([sdim.name for sdim in sdims]) |
There was a problem hiding this comment.
Was thinking it looks like the set intersection symbol if you squint at it?
|
|
||
| name = '_n_'.join([sdim.name for sdim in sdims]) | ||
| try: | ||
| [parent] = set([sdim.parent for sdim in sdims]) |
There was a problem hiding this comment.
{self.parent}.union(*[sdim.parent for sdim in sdims])
There was a problem hiding this comment.
Note that this is both getting the parent and checking they all share a parent, since the unpack will fail with ValueError if the set does not contain one item
|
|
||
| if all([sdim.is_left for sdim in sdims]): | ||
| # Only left SubDimensions results in left SubDimension | ||
| thickness = sympy.Min(*[sdim.thickness.left[1] |
| local=local) | ||
| elif all([sdim.is_right for sdim in sdims]): | ||
| # Only right SubDimensions results in right SubDimension | ||
| thickness = sympy.Min(*[sdim.thickness.right[1] |
| # will always be middle | ||
| width = parent.symbolic_max - parent.symbolic_min + 1 | ||
|
|
||
| def get_left_thickness(sdim): |
There was a problem hiding this comment.
define as instance attribute outside, and then just self/sdim.right_thickness , no need for "get"
Tests to come. Union is more complex and less urgent so was going to leave for now |
Introduce the ability to form a new
SubDimensionas the intersection of multipleSubDimensions