Skip to content

FillEmptyRows-16 operator specification #28046

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
May 7, 2025
Merged

FillEmptyRows-16 operator specification #28046

merged 19 commits into from
May 7, 2025

Conversation

p-wysocki
Copy link
Contributor

@p-wysocki p-wysocki commented Dec 12, 2024 •
edited
Loading

@p-wysocki
Initial draft
7d810fc 
Signed-off-by: p-wysocki <[email protected]>
@github-actions github-actions bot added the category: docs OpenVINO documentation label Dec 12, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR will be closed in a week because of 2 weeks of no activity.

@github-actions github-actions bot added the Stale label Feb 13, 2025
@github-actions GitHub Actions
Copy link
Contributor

This PR was closed because it has been stalled for 2 week with no activity.

@github-actions github-actions bot closed this Feb 20, 2025
@p-wysocki
Merge branch 'master' of https://github.com/openvinotoolkit/openvino …
e255fc9 
…into empty_rows_spec

Signed-off-by: p-wysocki <[email protected]>
@p-wysocki
Update rst
f5f4d8e 
Signed-off-by: p-wysocki <[email protected]>
@p-wysocki p-wysocki reopened this Mar 25, 2025
@p-wysocki p-wysocki marked this pull request as ready for review March 25, 2025 16:21
@p-wysocki p-wysocki requested a review from a team as a code owner March 25, 2025 16:21
@p-wysocki p-wysocki requested review from tsavina and removed request for a team March 25, 2025 16:21
@github-actions github-actions bot removed the Stale label Mar 26, 2025
mitruska
mitruska reviewed Apr 8, 2025
View reviewed changes
...ocumentation/openvino-ir-format/operation-sets/operation-specs/sparse/fill-empty-rows-16.rst Outdated
Comment on lines 46 to 52
* **1**: ``output_indices`` 2D tensor of type *T_IDX* indicating the positions at which ``output_values`` are placed in the sparse tensor.
It is of shape ``[M', N]``, where:

* ``M'`` is the length of the updated ``output_values``.
* ``N`` is equal to the rank of ``dense_shape``.
* **2**: ``output_values`` 1D tensor containing the values of type *T* to be inserted at the specified indices.
* **3**: ``empty_row_indicator`` 1D tensor of type ``boolean`` indicating True for rows which were empty before executing the operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

To have the whole sparse tensor representation, shouldn't the dense_shape be an output as well?
On the other hand, assuming it's the same as the input dense_shape maybe it's reduntant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tf.raw_ops.SparseFillEmptyRows doesn't return the output SparseTensor at all according to https://www.tensorflow.org/api_docs/python/tf/raw_ops/SparseFillEmptyRows#returns. Do you think we should have it?

That's also interesting because there's tf.sparse.fill_empty_rows and then there's tf.raw_ops.SparseFillEmptyRows, which have quite different outputs:

https://www.tensorflow.org/api_docs/python/tf/sparse/fill_empty_rows
https://www.tensorflow.org/api_docs/python/tf/raw_ops/SparseFillEmptyRows

However the BST model I received is using the raw_ops one and in the graph the SparseTensor is not returned.

rkazants
rkazants reviewed Apr 9, 2025
View reviewed changes
...ocumentation/openvino-ir-format/operation-sets/operation-specs/sparse/fill-empty-rows-16.rst Outdated
**Outputs**:

* **1**: ``output_indices`` 2D tensor of type *T_IDX* indicating the positions at which ``output_values`` are placed in the sparse tensor.
It is of shape ``[M', N]``, where:
Copy link
Member

Choose a reason for hiding this comment

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

do we really need generalization of tf.raw_ops.SparseFillEmptyRows? May be to consider only 2D sparse tensor that is only needed by TF?
I think ND case can be converged to 2D case by reshape

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If only 2D is needed, let's limit it. Applied.

rkazants
rkazants reviewed Apr 9, 2025
View reviewed changes
Copy link
Member

@rkazants rkazants left a comment

Choose a reason for hiding this comment

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

btw, will we support the same operation for string sparse tensor?

p-wysocki and others added 3 commits April 11, 2025 12:41
@p-wysocki @mitruska
Update docs/articles_en/documentation/openvino-ir-format/operation-se…
2816e14 
…ts/operation-specs/sparse/fill-empty-rows-16.rst

Co-authored-by: Katarzyna Mitrus <[email protected]>
@p-wysocki @rkazants
Update docs/articles_en/documentation/openvino-ir-format/operation-se…
8415489 
…ts/operation-specs/sparse/fill-empty-rows-16.rst

Co-authored-by: Roman Kazantsev <[email protected]>
@p-wysocki @mitruska
Update docs/articles_en/documentation/openvino-ir-format/operation-se…
06c0dd0 
…ts/operation-specs/sparse/fill-empty-rows-16.rst

Co-authored-by: Katarzyna Mitrus <[email protected]>
@p-wysocki
Copy link
Contributor Author

btw, will we support the same operation for string sparse tensor?

Yes, it's planned but as a separate operator. It was a decision based on how complex an all-in-one implementation would be - you can peek at the initial commit in the PR (7d810fc)

@p-wysocki p-wysocki requested review from mitruska and rkazants April 11, 2025 11:04
p-wysocki and others added 2 commits April 18, 2025 13:03
p-wysocki and others added 2 commits April 18, 2025 14:28
@p-wysocki
Copy link
Contributor Author

This PR needs to be force merged as agreed on with @akashchi - the failing CI is a bug tracked in CVS-166243.

@p-wysocki
Update docs/articles_en/documentation/openvino-ir-format/operation-se…
bdc85a0 
…ts/operation-specs/sparse/fill-empty-rows-16.rst
@p-wysocki p-wysocki mentioned this pull request May 7, 2025
Merged
@rkazants rkazants added this pull request to the merge queue May 7, 2025
github-merge-queue bot pushed a commit that referenced this pull request May 7, 2025
@p-wysocki
Add SparseFillEmptyRows-16 to Core (#30191)
6b2ca78 
Related PRs:
 - #28046
 - #30307

### Tickets:
 - CVS-158909

---------

Signed-off-by: p-wysocki <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 7, 2025
@rkazants rkazants added this pull request to the merge queue May 7, 2025
Merged via the queue into openvinotoolkit:master with commit 2b9a1c8 May 7, 2025
148 checks passed
github-merge-queue bot pushed a commit that referenced this pull request May 8, 2025
@p-wysocki
Add SparseFillEmptyRows-16 reference implementation (#30307)
b85136f 
### Details:
- This PR is built on top of
#30191 and contains its
commits, so it's better to wait for its merge before reviewing
 - Add reference implementation and tests

### Related PRs:
 - #28046
 - #30191

### Tickets:
 - CVS-158910

---------

Signed-off-by: p-wysocki <[email protected]>
This was referenced May 8, 2025
Merged
Merged
Merged
github-merge-queue bot pushed a commit that referenced this pull request May 16, 2025
@p-wysocki
Extend CPU Plugin with reference implementation of `SparseFillEmptyRo…
4eda36b 
…ws-16` (#30474)

### Related PRs:
 - #28046
 - #30191
 - #30307
 - #30487
 - #30504

### Tickets:
 - CVS-158911

---------

Signed-off-by: p-wysocki <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request May 20, 2025
@p-wysocki @mlukasze
[PyOV] Extend Python API with SparseFillEmptyRows-16 (#30487)
b6404a3 
### Related PRs:
 - #28046
 - #30191
 - #30307
 - #30474
 - #30504

### Details:
- This PR has the CPU PR merged into it, best to wait for its merge
before review

### Tickets:
 - CVS-158912

---------

Signed-off-by: p-wysocki <[email protected]>
Co-authored-by: Michal Lukaszewski <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request May 20, 2025
@p-wysocki @mlukasze
[TF FE] Extend TF FE with ov::op::SparseFillEmptyRows-16 (#30504)
c98dd8f 
### Details:
- This PR has the CPU PR merged into it, it's best to wait for its merge
before reviewing

### Related PRs:
 - #28046
 - #30191
 - #30307
 - #30487
 - #30474

### Tickets:
 - CVS-167469

---------

Signed-off-by: p-wysocki <[email protected]>
Co-authored-by: Michal Lukaszewski <[email protected]>
@p-wysocki p-wysocki mentioned this pull request Jun 2, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkazants rkazants rkazants approved these changes

@mitruska mitruska mitruska approved these changes

@tsavina tsavina tsavina approved these changes

@mmikolajcz mmikolajcz Awaiting requested review from mmikolajcz

Assignees
No one assigned
Labels
category: docs OpenVINO documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@p-wysocki @mitruska @rkazants @tsavina @mlukasze