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

Numpy fill accepts also variables #1420

Merged
merged 17 commits into from Nov 17, 2023

Conversation

philip-paul-mueller
Copy link
Collaborator

This PR is for addressing issue #1389.

It is now a bit nicer, less compact but much more readable.
Further, one function should now be much clearer how it works and it now guarantees that the same Python resolution order is used.
This changes makes it possible that the `fill()` function accepts variables.
However, this particular commit only allows variables that where passed as arguments, at least in some sense, see the discussion of the issue.

This is a first step to address issue [spcl#1389](spcl#1389).
@philip-paul-mueller philip-paul-mueller marked this pull request as draft November 1, 2023 10:15
@philip-paul-mueller
Copy link
Collaborator Author

philip-paul-mueller commented Nov 1, 2023

While this PR allows for code such as:

import dace
import numpy as np

N = dace.symbol("N", dace.int32)

@dace.program
def fill_dace(A: dace.float64[N, N], value: dace.float64):
    A.fill(value)

it does currently not allows for code such as:

@dace.program
def fill_dace_not_working(A: dace.float64[N, N], value: dace.float64):
    A.fill(value + 1)

The reason is that the argument to fill is an expression and not a scalar, however, it could be evaluated to a scalar.

What is funny though is that the code:

@dace.program
def fill_dace_does_also_not_work(A: dace.float64[N, N], value: dace.float64):
    value2 = value + 1
    A.fill(value2)

will also not work but:

@dace.program
def fill_dace_will_work_but_why(A: dace.float64[N, N], value: dace.float64):
    value2 = value + 1
    A.fill(value2)
    return value2

will work, for some strange reason and I have no idea why?

Disturbingly this:

@dace.program
def fill_dace_why_god_does_this_not_work(A: dace.float64[N, N], value: dace.float64):
    value2 = value + 1
    A.fill(value2)
    return value

does not work, and I have no idea why?

For more see samples/simple/numpy_fill.py

Note the programm shows the current state and not what is desired.
The logic of the check has changed, but I forget to change the check itself.

Why was that thing working before?
@philip-paul-mueller
Copy link
Collaborator Author

Current State:
To summarize the current state:

  • A.fill(value): Where value is an argument -> Works
  • A.fill(Expr): Where Expr is an expression, such as value + 1 -> Does not work (probably need some big changes)
  • c = value + 2; A.fill(c): Where value is an argument -> Does not work (This is probably a bug in the Python frontend, see above)

…ocaly defined variable.

It Addresses the last point mentioned in the discussion of PR [spcl#1420](spcl#1420 (comment)).
However, there is still a problem in case an expression is passed as argument.
Now the problem is that there is some shadowing.
It is no longer based on `_elementwise()` but is now a dedicated implementation, which is based on `full()`.
@philip-paul-mueller
Copy link
Collaborator Author

Commit a29ae4a should now solve everything of the above list.

@philip-paul-mueller philip-paul-mueller self-assigned this Nov 2, 2023
@philip-paul-mueller philip-paul-mueller marked this pull request as ready for review November 3, 2023 06:35
@philip-paul-mueller philip-paul-mueller changed the title WIP: Numpy fill accepts also variables Numpy fill accepts also variables Nov 7, 2023
@BenWeber42 BenWeber42 linked an issue Nov 15, 2023 that may be closed by this pull request
Added Ben's Suggestions.

Co-authored-by: BenWeber42 <dev.ben.weber@gmail.com>
Co-authored-by: BenWeber42 <dev.ben.weber@gmail.com>
@BenWeber42 BenWeber42 merged commit 40ed438 into spcl:master Nov 17, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Numpy fill accepts only numbers
3 participants