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

More NumPy operation implementations #1498

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

More NumPy operation implementations #1498

wants to merge 21 commits into from

Conversation

tbennun
Copy link
Collaborator

@tbennun tbennun commented Jan 7, 2024

  • Concatenation and stacking (numpy.concatenate, numpy.stack, and their variants)
  • numpy.linspace
  • Fix nested attribute parsing (Fixes Incorrect parsing of parentheses in DaCe programs #1295)
  • numpy.clip
  • numpy.split and its variants
  • numpy.full variants (zeros, ones, etc.) with a single value for shape (np.zeros(N))
  • NumPy-compatible numpy.arange dtype inference
  • numpy.fft.{fft, ifft}

@tbennun tbennun marked this pull request as ready for review January 8, 2024 09:27
@tbennun tbennun added the in the merge queue waiting for CI to work again label Jan 14, 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.

Looks fine

@@ -4426,7 +4430,7 @@ def visit_Call(self, node: ast.Call, create_callbacks=False):
arg = self.scope_vars[modname]
else:
# Fallback to (name, object)
arg = (modname, self.defined[modname])
arg = modname
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 change handled already in the code somehow, i.e., is self.defined queried on demand?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It’s a code path that we didn’t cover before in coverage. The rest of the code assumes a string return type rather than a tuple, and crashes somewhere else.

result = self._visitname(name, node)
result = self.visit(node.value)
if isinstance(result, (tuple, list, dict)):
if len(result) > 1:
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 the fix for attributes on expressions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly

@@ -527,30 +535,43 @@ def _arange(pv: ProgramVisitor, sdfg: SDFG, state: SDFGState, *args, **kwargs):
else:
start, stop, step = args

if isinstance(start, str):
Copy link
Contributor

Choose a reason for hiding this comment

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

The underlying issue here is data-dependent array shape?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, though generally solvable with a symbol assignment and another state, for now at least symbols and constants should be supported.

if not isinstance(shape[0], Number) and ('dtype' not in kwargs or kwargs['dtype'] == None):
raise NotImplementedError("The current implementation of numpy.arange requires that the output dtype is given "
"when at least one of (start, stop, step) is symbolic.")
# Infer dtype from input arguments
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous code looks suspicious. There is probably a reason we didn't fall back to using _result_type. Probably arange only works with integers or something. I guess it is not important if the code is "correct" (numpy-wise), but it is good to keep in mind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It may have been written before/without knowledge of _result_type

elif dtype == dace.complex128:
func = "zomatcopy"
alpha = "dace::blas::BlasConstants::Get().Complex128Pone()"
cast = '(double*)'
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the impression that this has been fixed in another PR, but maybe it is stuck in the queue. Just noting it in case merge issues appear later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was fixed for some BLAS operators but not others. Also there is some tensor transpose PR but I think it’s GPU focused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in the merge queue waiting for CI to work again
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect parsing of parentheses in DaCe programs
2 participants