Skip to content

Added Operations and tests#92

Open
Felipetoledo4815 wants to merge 23 commits into
dlshriver:developfrom
Felipetoledo4815:develop
Open

Added Operations and tests#92
Felipetoledo4815 wants to merge 23 commits into
dlshriver:developfrom
Felipetoledo4815:develop

Conversation

@Felipetoledo4815

Copy link
Copy Markdown
Contributor

No description provided.

@codecov

codecov Bot commented Jun 30, 2022

Copy link
Copy Markdown

Codecov Report

Merging #92 (965383e) into develop (c9c5899) will decrease coverage by 0.25%.
The diff coverage is 81.28%.

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

@@             Coverage Diff             @@
##           develop      #92      +/-   ##
===========================================
- Coverage    89.64%   89.38%   -0.26%     
===========================================
  Files          138      138              
  Lines         8016     8199     +183     
  Branches      1491     1516      +25     
===========================================
+ Hits          7186     7329     +143     
- Misses         602      628      +26     
- Partials       228      242      +14     
Impacted Files Coverage Δ
dnnv/nn/operations/tensor.py 78.05% <71.15%> (-4.07%) ⬇️
dnnv/nn/converters/tensorflow.py 94.00% <77.77%> (-1.64%) ⬇️
dnnv/nn/converters/onnx.py 93.29% <88.23%> (-0.83%) ⬇️
dnnv/nn/visitors.py 98.84% <100.00%> (+0.07%) ⬆️
...nnv/properties/transformers/propagate_constants.py 94.93% <100.00%> (+0.02%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dlshriver dlshriver left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you for the pull request, these look like useful additions. I have several requested changes before merging that I'd like to see addressed. There is also currently a test failure that does not seem to be due to your changes that I'll look into to see if we can get the test suite to pass before merging as well.

Comment thread dnnv/nn/operations/tensor.py Outdated
Comment thread dnnv/nn/operations/tensor.py Outdated
Comment thread dnnv/nn/operations/tensor.py Outdated
Comment thread dnnv/nn/operations/tensor.py Outdated
Comment thread dnnv/nn/operations/tensor.py Outdated
from dnnv.nn.converters.onnx import *
from dnnv.nn.operations import *


Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you also add tests where each input to Squeeze can be an Input operation to ensure that the operation works with non-concrete input values? You can duplicate one of the test cases below and use input operations instead of concrete values to construct the op graph, and then pass the required values in when you call the op graph to get the outputs.

from dnnv.nn.converters.tensorflow import *
from dnnv.nn.operations import *


Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you also add tests where each input to Clip can be an Input operation to ensure that the operation works with non-concrete input values? You can duplicate one of the test cases below and use input operations instead of concrete values to construct the op graph, and then pass the required values in when you call the op graph to get the outputs.

from dnnv.nn.converters.tensorflow import *
from dnnv.nn.operations import *


Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you also add tests where each input to ReduceL2 can be an Input operation to ensure that the operation works with non-concrete input values? You can duplicate one of the test cases below and use input operations instead of concrete values to construct the op graph, and then pass the required values in when you call the op graph to get the outputs.

from dnnv.nn.converters.tensorflow import *
from dnnv.nn.operations import *


Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you also add tests where each input to Squeeze can be an Input operation to ensure that the operation works with non-concrete input values? You can duplicate one of the test cases below and use input operations instead of concrete values to construct the op graph, and then pass the required values in when you call the op graph to get the outputs.

from dnnv.nn.converters.tensorflow import *
from dnnv.nn.operations import *


Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you also add tests where each input to Upsample can be an Input operation to ensure that the operation works with non-concrete input values? You can duplicate one of the test cases below and use input operations instead of concrete values to construct the op graph, and then pass the required values in when you call the op graph to get the outputs.

@dlshriver dlshriver self-assigned this Jul 6, 2022
@dlshriver dlshriver added the enhancement New feature or request label Jul 6, 2022
@dlshriver

Copy link
Copy Markdown
Owner

Would you also add/edit your description of the pull request please, so that we can keep documentation on the changes that this should be making.

Felipetoledo4815 and others added 7 commits July 7, 2022 11:00
Co-authored-by: David Shriver <davidshriver@outlook.com>
Co-authored-by: David Shriver <davidshriver@outlook.com>
Co-authored-by: David Shriver <davidshriver@outlook.com>
Co-authored-by: David Shriver <davidshriver@outlook.com>
Co-authored-by: David Shriver <davidshriver@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants