compiler: Fix placement of ConditionalDimension in subdomain #2050

georgebisbas · 2023-01-23T18:26:57Z

No description provided.

codecov · 2023-01-23T18:34:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.75%. Comparing base (cafebc1) to head (c35a0c1).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2050      +/-   ##
==========================================
+ Coverage   86.73%   86.75%   +0.01%     
==========================================
  Files         233      233              
  Lines       43648    43707      +59     
  Branches     8077     8077              
==========================================
+ Hits        37859    37916      +57     
- Misses       5079     5080       +1     
- Partials      710      711       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

georgebisbas · 2023-01-23T18:51:29Z

devito/ir/clusters/algorithms.py

+                    # If `cd` uses more dimensions than the ispace,
+                    # stay under parent
+                    if (not dims.issubset(set(c.ispace.dimensions)) and
+                       cd.parent in dims):
+                        k = cd.parent
+                    else:
+                        k = max(dims, default=d, key=lambda i: c.ispace.index(i))


Solution 2 with False flag

# If `cd` uses, as condition, an arbitrary SymPy expression, then # we must ensure to nest it inside the last of the Dimensions # appearing in `cd.condition` if cd._factor is not None: k = d else: dims = pull_dims(cd.condition, flag=False) k = max(dims, default=d, key=lambda i: c.ispace.index(i))

yeah at first glance it would appear a bit convoluted

Do you also consider the second solution not good enough?

It's not clear why passing flag=False would fix the issue here?

FabioLuporini

Do the new tests actually fail under master?

FabioLuporini · 2023-01-30T10:54:37Z

tests/test_subdomains.py

+
+        assert_structure(op, ['i1x'], 'i1x')
+
+    def test_condition_w_subdomain_II(self):


usually we use v0, v1, v2... instead of I, II, III

FabioLuporini · 2023-01-30T10:55:29Z

tests/test_subdomains.py

+        condition = Lt(sdf[mid.dimensions[0]], 1)
+
+        ci = ConditionalDimension(name='ci', condition=condition,
+                                  parent=mid.dimensions[0])


mid.dimensions[0] is a root dimension already, do we actually need this test?

IOW, would this test actually fail in current master?

#2050 (comment)

georgebisbas · 2023-01-30T12:02:23Z

Do the new tests actually fail under master?

Yes, they fail in the current master with the python-land exception
(I guess the same you think of here: https://devitocodes.slack.com/archives/C7JMLMSG0/p1674301184495359?thread_ts=1674231704.845469&cid=C7JMLMSG0)

georgebisbas · 2023-03-24T17:38:37Z

@FabioLuporini plans for this?

FabioLuporini · 2023-03-25T09:49:02Z

@georgebisbas plan imho is to find a neater/simpler/better way

mloubout · 2023-09-21T13:31:52Z

What's the status on this?

georgebisbas · 2023-10-11T15:27:51Z

@FabioLuporini requested a better solution on this.
Any ides?

mloubout · 2023-10-11T15:30:16Z

As per my comment above, I don't think it's an issue anymore and should work without the change

georgebisbas · 2023-10-11T16:33:50Z

@mloubout I cannot see any comment of yours (?)

pytest tests/test_subdomains.py::TestSubDomain_w_condition

I tried to run the tests and they fail

EdCaunt · 2024-04-17T15:14:30Z

Can we resurrect this? It fixes an issue which still exists (I accidentally replicated the fix in #2357)

FabioLuporini

This is now good to go imho

mloubout

Much cleaner than before, one minro comment

mloubout · 2024-04-19T12:04:41Z

devito/ir/clusters/algorithms.py

@@ -238,7 +238,7 @@ def guard(clusters):
                if cd._factor is not None:
                    k = d
                else:
-                    dims = pull_dims(cd.condition)
+                    dims = pull_dims(cd.condition, flag=False)


I think this should be flag=cd.indirect for the case where the conditional is used as the indexing dimension

georgebisbas · 2024-04-25T10:51:20Z

tests/test_subdomains.py

+
+        assert_structure(op, ['xy'], 'xy')
+
+    def test_condition_w_subdomain_v2(self):


@mloubout could you help sketching a test about what you mention as the conditional being the indexing dimension?
or at least show me an example with some pseducode?

Ok sorry after bit of digging, forgot this algorithm section is only for non indirect (see line 223 in algorithms.py)

GTG

ah right, this is what happens if I have long time to see a PR

mloubout · 2024-04-25T11:47:44Z

tests/test_subdomains.py

+
+        assert_structure(op, ['xy'], 'xy')
+
+    def test_condition_w_subdomain_v2(self):


Ok sorry after bit of digging, forgot this algorithm section is only for non indirect (see line 223 in algorithms.py)

GTG

…t and subdomain case

georgebisbas added the compiler label Jan 23, 2023

georgebisbas self-assigned this Jan 23, 2023

georgebisbas changed the title ~~[RFC] compiler: Fix issue 2049~~ [RFC] compiler: Fix placement of ConditionalDimension in subdomain Jan 23, 2023

georgebisbas force-pushed the cond_subd branch from a5e6192 to e6d6e4f Compare January 23, 2023 18:38

georgebisbas commented Jan 23, 2023

View reviewed changes

FabioLuporini reviewed Jan 30, 2023

View reviewed changes

georgebisbas force-pushed the cond_subd branch from e6d6e4f to 187bdb6 Compare March 17, 2023 19:06

georgebisbas requested review from FabioLuporini and mloubout March 17, 2023 19:10

georgebisbas changed the title ~~[RFC] compiler: Fix placement of ConditionalDimension in subdomain~~ compiler: Fix placement of ConditionalDimension in subdomain Mar 17, 2023

georgebisbas added the bug-py-minor label Mar 17, 2023

georgebisbas force-pushed the cond_subd branch 2 times, most recently from 02a8511 to 9eb007d Compare May 17, 2023 15:29

georgebisbas force-pushed the cond_subd branch from 9eb007d to d36ba9b Compare September 4, 2023 15:33

georgebisbas force-pushed the cond_subd branch from d36ba9b to 6cc3a5a Compare October 11, 2023 15:27

georgebisbas force-pushed the cond_subd branch from 6cc3a5a to ac53256 Compare April 17, 2024 16:10

FabioLuporini approved these changes Apr 18, 2024

View reviewed changes

EdCaunt mentioned this pull request Apr 18, 2024

compiler: Fix ConditionalDimensions on SubDomains #2357

Closed

EdCaunt approved these changes Apr 18, 2024

View reviewed changes

mloubout reviewed Apr 18, 2024

View reviewed changes

georgebisbas linked an issue Apr 19, 2024 that may be closed by this pull request

Combination of Subdomain and ConditionalDimension #2049

Closed

mloubout reviewed Apr 19, 2024

View reviewed changes

georgebisbas commented Apr 25, 2024

View reviewed changes

mloubout approved these changes Apr 25, 2024

View reviewed changes

georgebisbas force-pushed the cond_subd branch 2 times, most recently from e7fcd2d to ba08fa7 Compare April 25, 2024 16:40

georgebisbas added 3 commits April 29, 2024 18:20

compiler: Fix placement of ConditionalDimension depending on cd.paren…

fa75c29

…t and subdomain case

compiler: Switch to alternative solution

4e89f58

tests: Fix after rebase

c35a0c1

mloubout force-pushed the cond_subd branch from ba08fa7 to c35a0c1 Compare April 29, 2024 22:20

mloubout merged commit daf782e into master Apr 30, 2024
31 checks passed

mloubout deleted the cond_subd branch April 30, 2024 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Fix placement of ConditionalDimension in subdomain #2050

compiler: Fix placement of ConditionalDimension in subdomain #2050

georgebisbas commented Jan 23, 2023

codecov bot commented Jan 23, 2023 •

edited

Loading

georgebisbas Jan 23, 2023

FabioLuporini Jan 24, 2023

georgebisbas Apr 3, 2023

FabioLuporini Apr 4, 2023

FabioLuporini left a comment

FabioLuporini Jan 30, 2023

FabioLuporini Jan 30, 2023

georgebisbas Mar 17, 2023

georgebisbas commented Jan 30, 2023

georgebisbas commented Mar 24, 2023

FabioLuporini commented Mar 25, 2023

mloubout commented Sep 21, 2023

georgebisbas commented Oct 11, 2023

mloubout commented Oct 11, 2023

georgebisbas commented Oct 11, 2023

EdCaunt commented Apr 17, 2024

FabioLuporini left a comment

mloubout left a comment

mloubout Apr 19, 2024

georgebisbas Apr 25, 2024

mloubout Apr 25, 2024

georgebisbas Apr 25, 2024

mloubout Apr 25, 2024


		assert_structure(op, ['i1x'], 'i1x')

		def test_condition_w_subdomain_II(self):


		assert_structure(op, ['xy'], 'xy')

		def test_condition_w_subdomain_v2(self):

compiler: Fix placement of ConditionalDimension in subdomain #2050

compiler: Fix placement of ConditionalDimension in subdomain #2050

Conversation

georgebisbas commented Jan 23, 2023

codecov bot commented Jan 23, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgebisbas commented Jan 30, 2023

georgebisbas commented Mar 24, 2023

FabioLuporini commented Mar 25, 2023

mloubout commented Sep 21, 2023

georgebisbas commented Oct 11, 2023

mloubout commented Oct 11, 2023

georgebisbas commented Oct 11, 2023

EdCaunt commented Apr 17, 2024

FabioLuporini left a comment

Choose a reason for hiding this comment

mloubout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 23, 2023 •

edited

Loading