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

Add a SubrangeMapper helper class which maps a _subrange_ of src range to its counterpart in dst range, if possible. #1702

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

pratyai
Copy link
Collaborator

@pratyai pratyai commented Oct 23, 2024

Equipped with a src and a dst range of equal volumes but possibly different shapes or even dimensions (i.e.,
there is an 1-to-1 correspondence between the elements of src and dst), maps a subrange of src to its
counterpart in dst, if possible (which is not always the case).

It remains dead-code in this PR and not used anywhere at all. But it should help with correctly reshaping memlet subsets in a following PR (which will aim to fix RedundantArray transform by simplying it to something like this: https://github.com/pratyai/dace/blob/be571d21aa8abbc465daa11d04941040fbf93401/dace/transformation/dataflow/redundant_array.py#L426-L480).

@pratyai pratyai marked this pull request as ready for review October 23, 2024 13:15
@pratyai pratyai requested a review from alexnick83 October 23, 2024 13:17
@pratyai pratyai marked this pull request as draft October 24, 2024 11:19
@pratyai pratyai force-pushed the volume branch 2 times, most recently from 3db98e1 to d368670 Compare October 24, 2024 17:25
@pratyai pratyai added the no-ci Do not run any CI or actions for this PR label Oct 24, 2024
Copy link
Contributor

@alexnick83 alexnick83 left a comment

Choose a reason for hiding this comment

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

SubrangeMapper.map looks a bit convoluted but seems like a useful method overall. I will re-review when the tests pass.

dace/subsets.py Outdated Show resolved Hide resolved
dace/subsets.py Outdated
@@ -895,6 +909,10 @@ def size(self):
def size_exact(self):
return self.size()

def volume_exact(self) -> int:
""" Returns the total number of elements in all dimenssions together. """
return reduce(operator.mul, self.size_exact())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this compatible with all supported Python versions? Note that we also have another method that does the same in dace.data:

def _prod(sequence):

Maybe it is a good idea to move (one of them) to a utility method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I can tell, it does work for 3.7 and 3.12. Replaced with lambda function regardless, since that's how _prod works.

I don't think it's necessary to make reduce(lambda a, b : a * b, sequence) itself an utility function. It's really short, easy to see what it does. I've added the volume_exact() function to just give that operation a meaningful name (i.e. why it does that). But of course, if you rather have it separately, I can move it too.

Its functionality
===
Equipped with a `src` and a `dst` range of equal volumes but possibly different shapes or even dimensions (i.e.,
there is an 1-to-1 correspondence between the elements of `src` and `dst`), maps a subrange of `src` to its
counterpart in `dst`, if possible.

Note that such subrange-to-subrange mapping may not always exist.
@pratyai pratyai force-pushed the volume branch 2 times, most recently from 8cc57a6 to 92b82a1 Compare October 25, 2024 13:20
@pratyai
Copy link
Collaborator Author

pratyai commented Oct 25, 2024

SubrangeMapper.map looks a bit convoluted but seems like a useful method overall.

I can try and rephrase the comments. If you have any suggestion on phrasing, please let me know.
The gist is: We were writing to some range in A, and then some superset of that range was written out to B. Now that we're removing A and will directly write to B instead, what should be the range for the new write.
I'll also do a cleanup later with the comments, removing any garbage print statements etc.

I will re-review when the tests pass.

The unit tests pass. For the CI tests, please check the last commit to see what I'm aiming for (note: this commit is incomplete, but is there just to demonstrate that with a few additional bug fixes, redundant array can be simplified a lot while at the same time correcting its behaviour).

I'll try to trigger a CI run later.

@pratyai pratyai changed the title Add a SubrangeMapper helper class. Add a SubrangeMapper helper class which maps a _subrange_ of src range to its counterpart in dst range, if possible. Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-ci Do not run any CI or actions for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants