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

Implement ConvolveOp #1485

Merged
merged 1 commit into from
Apr 23, 2023
Merged

Implement ConvolveOp #1485

merged 1 commit into from
Apr 23, 2023

Conversation

Smit-create
Copy link
Member

@Smit-create Smit-create commented Mar 26, 2023

Fixes #1223
Here are a few important guidelines and requirements to check before your PR can be merged:

  • There is an informative high-level description of the changes.
  • The description and/or commit message(s) references the relevant GitHub issue(s).
  • pre-commit is installed and set up.
  • The commit messages follow these guidelines.
  • The commits correspond to relevant logical changes, and there are no commits that fix changes introduced by other commits in the same branch/BR.
  • There are tests covering the changes introduced in the PR.

Don't worry, your PR doesn't need to be in perfect order to submit it. As development progresses and/or reviewers request changes, you can always rewrite the history of your feature/PR branches.

If your PR is an ongoing effort and you would like to involve us in the process, simply make it a draft PR.

@codecov
Copy link

codecov bot commented Mar 26, 2023

Codecov Report

Merging #1485 (cc6f9f2) into main (fa2bbaf) will increase coverage by 0.01%.
The diff coverage is 92.00%.

❗ Current head cc6f9f2 differs from pull request most recent head f64018b. Consider uploading reports for the commit f64018b to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1485      +/-   ##
==========================================
+ Coverage   75.06%   75.08%   +0.01%     
==========================================
  Files         194      194              
  Lines       50101    50151      +50     
  Branches    12097    12107      +10     
==========================================
+ Hits        37610    37656      +46     
- Misses      10170    10172       +2     
- Partials     2321     2323       +2     
Impacted Files Coverage Δ
aesara/tensor/basic.py 90.05% <92.00%> (+0.06%) ⬆️

Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

Looks good! Convolve._get_output_shape needs more coverage before we can merge, but that might be it.

@Smit-create Smit-create marked this pull request as ready for review April 9, 2023 06:29
tests/tensor/test_basic.py Outdated Show resolved Hide resolved
tests/tensor/test_basic.py Outdated Show resolved Hide resolved
tests/tensor/test_basic.py Outdated Show resolved Hide resolved
@Smit-create
Copy link
Member Author

I have tried to accommodate the fix that you suggested. I have the following issues:

  1. The CI says a failing code-style test but that passes for me locally:
% pre-commit run --show-diff-on-failure --color=always --all-files
debug statements (python)................................................Passed
check for merge conflicts................................................Passed
pyupgrade................................................................Passed
black....................................................................Passed
flake8...................................................................Passed
isort....................................................................Passed
autoflake................................................................Passed
mypy.....................................................................Passed
  1. Running the tests, there is a failure on the C-compilation side on macOS:
Traceback (most recent call last):
  File "/Users/thebigbool/repos/aesara/b.py", line 160, in <module>
    aesara_sol = convolve(a, v, mode=mode).eval()
  File "/Users/thebigbool/repos/aesara/aesara/graph/basic.py", line 605, in eval
    self._fn_cache[inputs] = function(inputs, self)
  File "/Users/thebigbool/repos/aesara/aesara/compile/function/__init__.py", line 317, in function
    fn = pfunc(
  File "/Users/thebigbool/repos/aesara/aesara/compile/function/pfunc.py", line 367, in pfunc
    return orig_function(
  File "/Users/thebigbool/repos/aesara/aesara/compile/function/types.py", line 1815, in orig_function
    fn = m.create(defaults)
  File "/Users/thebigbool/repos/aesara/aesara/compile/function/types.py", line 1708, in create
    _fn, _i, _o = self.linker.make_thunk(
  File "/Users/thebigbool/repos/aesara/aesara/link/basic.py", line 254, in make_thunk
    return self.make_all(
  File "/Users/thebigbool/repos/aesara/aesara/link/vm.py", line 1252, in make_all
    raise_with_op(fgraph, node)
  File "/Users/thebigbool/repos/aesara/aesara/link/utils.py", line 533, in raise_with_op
    raise exc_value.with_traceback(exc_trace)
  File "/Users/thebigbool/repos/aesara/aesara/link/vm.py", line 1243, in make_all
    node.op.make_thunk(node, storage_map, compute_map, [], impl=impl)
  File "/Users/thebigbool/repos/aesara/aesara/link/c/op.py", line 131, in make_thunk
    return self.make_c_thunk(node, storage_map, compute_map, no_recycling)
  File "/Users/thebigbool/repos/aesara/aesara/link/c/op.py", line 96, in make_c_thunk
    outputs = cl.make_thunk(
  File "/Users/thebigbool/repos/aesara/aesara/link/c/basic.py", line 1200, in make_thunk
    cthunk, module, in_storage, out_storage, error_storage = self.__compile__(
  File "/Users/thebigbool/repos/aesara/aesara/link/c/basic.py", line 1120, in __compile__
    thunk, module = self.cthunk_factory(
  File "/Users/thebigbool/repos/aesara/aesara/link/c/basic.py", line 1644, in cthunk_factory
    module = cache.module_from_key(key=key, lnk=self)
  File "/Users/thebigbool/repos/aesara/aesara/link/c/cmodule.py", line 1240, in module_from_key
    module = lnk.compile_cmodule(location)
  File "/Users/thebigbool/repos/aesara/aesara/link/c/basic.py", line 1543, in compile_cmodule
    module = c_compiler.compile_str(
  File "/Users/thebigbool/repos/aesara/aesara/link/c/cmodule.py", line 2654, in compile_str
    raise CompileError(
aesara.link.c.exceptions.CompileError: Compilation failed (return status=1):
/Users/thebigbool/opt/anaconda3/envs/aesara-dev-1/bin/clang++ -dynamiclib -g -O3 -fno-math-errno -Wno-unused-label -Wno-unused-variable -Wno-write-strings -DNPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION -m64 -fPIC -undefined dynamic_lookup -Wno-c++11-narrowing -I/Users/thebigbool/opt/anaconda3/envs/aesara-dev-1/lib/python3.10/site-packages/numpy/core/include -I/Users/thebigbool/opt/anaconda3/envs/aesara-dev-1/include/python3.10 -I/Users/thebigbool/repos/aesara/aesara/link/c/c_code -L/Users/thebigbool/opt/anaconda3/envs/aesara-dev-1/lib -fvisibility=hidden -o /Users/thebigbool/.aesara/compiledir_macOS-13.0-x86_64-i386-64bit-i386-3.10.8-64/tmpbb3yfwbq/mf6fae8a01551c3ac2d9a0aaba0ff89f2ec61ceb4a936a76263a7c5d4ea33e204.so /Users/thebigbool/.aesara/compiledir_macOS-13.0-x86_64-i386-64bit-i386-3.10.8-64/tmpbb3yfwbq/mod.cpp
ld: unsupported tapi file type '!tapi-tbd' in YAML file '/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/lib/libSystem.tbd' for architecture x86_64
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)

Apply node that caused the error: DeepCopyOp(TensorConstant{[ 0  0  1 .. 10 13 10]})
Toposort index: 0
Inputs types: [TensorType(int64, (8,))]

HINT: Use a linker other than the C linker to print the inputs' shapes and strides.
HINT: Re-running with most Aesara optimizations disabled could provide a back-trace showing when this node was created. This can be done by setting the Aesara flag 'optimizer=fast_compile'. If that does not work, Aesara optimizations can be disabled with 'optimizer=None'.
HINT: Use the Aesara flag `exception_verbosity=high` for a debug print-out and storage map footprint of this Apply node.
  1. Some of the tests do fail after removing the constant abstraction at the make_node level as seen in the following traceback, where it is unable to extract constant shape for mode="valid" and mode="same", where it should if the m, and n are already constant.
TypeError: __trunc__ returned non-Integral (type TensorVariable)

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/thebigbool/repos/aesara/b.py", line 160, in <module>
    aesara_sol = convolve(a, v, mode=mode).eval()
  File "/Users/thebigbool/repos/aesara/aesara/graph/basic.py", line 605, in eval
    self._fn_cache[inputs] = function(inputs, self)
  File "/Users/thebigbool/repos/aesara/aesara/compile/function/__init__.py", line 317, in function
    fn = pfunc(
  File "/Users/thebigbool/repos/aesara/aesara/compile/function/pfunc.py", line 367, in pfunc
    return orig_function(
  File "/Users/thebigbool/repos/aesara/aesara/compile/function/types.py", line 1803, in orig_function
    m = Maker(
  File "/Users/thebigbool/repos/aesara/aesara/compile/function/types.py", line 1574, in __init__
    self.prepare_fgraph(inputs, outputs, found_updates, fgraph, mode, profile)
  File "/Users/thebigbool/repos/aesara/aesara/compile/function/types.py", line 1434, in prepare_fgraph
    rewriter_profile = rewriter(fgraph)
  File "/Users/thebigbool/repos/aesara/aesara/graph/rewriting/basic.py", line 133, in __call__
    return self.rewrite(fgraph)
  File "/Users/thebigbool/repos/aesara/aesara/graph/rewriting/basic.py", line 128, in rewrite
    self.add_requirements(fgraph)
  File "/Users/thebigbool/repos/aesara/aesara/graph/rewriting/basic.py", line 349, in add_requirements
    rewrite.add_requirements(fgraph)
  File "/Users/thebigbool/repos/aesara/aesara/tensor/rewriting/shape.py", line 733, in add_requirements
    fgraph.attach_feature(ShapeFeature())
  File "/Users/thebigbool/repos/aesara/aesara/graph/fg.py", line 707, in attach_feature
    feature.on_attach(self)
  File "/Users/thebigbool/repos/aesara/aesara/tensor/rewriting/shape.py", line 525, in on_attach
    self.on_import(fgraph, node, reason="on_attach")
  File "/Users/thebigbool/repos/aesara/aesara/tensor/rewriting/shape.py", line 589, in on_import
    self.set_shape(r, s)
  File "/Users/thebigbool/repos/aesara/aesara/tensor/rewriting/shape.py", line 361, in set_shape
    shape_vars += (constant(r.type.shape[i], dtype="int64", ndim=0),)
  File "/Users/thebigbool/repos/aesara/aesara/tensor/basic.py", line 215, in constant
    x_ = aes.convert(x, dtype=dtype)
  File "/Users/thebigbool/repos/aesara/aesara/scalar/basic.py", line 245, in convert
    x_ = _asarray(x, dtype=dtype)
  File "/Users/thebigbool/repos/aesara/aesara/misc/safe_asarray.py", line 35, in _asarray
    rval = np.asarray(a, dtype=dtype, order=order)
ValueError: setting an array element with a sequence.

@brandonwillard
Copy link
Member

I just pushed a change that should fix some of those issues. It looks like we need to update the type hints (unrelated to this work), so I'll put in a separate PR for that shortly.

brandonwillard
brandonwillard previously approved these changes Apr 23, 2023
@Smit-create
Copy link
Member Author

Thanks @brandonwillard!

@brandonwillard brandonwillard merged commit ad7a8b7 into aesara-devs:main Apr 23, 2023
@Smit-create Smit-create deleted the i-1223 branch April 23, 2023 19:59
@maresb
Copy link
Contributor

maresb commented May 3, 2023

@Smit-create, let's open a new issue for

ld: unsupported tapi file type '!tapi-tbd' in YAML file '/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/lib/libSystem.tbd' for architecture x86_64

and including the output of mamba list. It sounds like it may be some sort of Conda-related issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request NumPy compatibility Op implementation Involves the implementation of an Op
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add at.convolve Op
3 participants