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

create new crate for core-ops #142

Merged
merged 19 commits into from
Oct 3, 2024
Merged

Conversation

jandremarais
Copy link
Contributor

Separate the core tensor operations from the core crate to a core-ops crate.

@jandremarais jandremarais marked this pull request as draft September 20, 2024 10:10
@jandremarais
Copy link
Contributor Author

@edgarriba do you want the operations currently defined in kornia-core/src/tensor.rs like add/mul/powi/etc to live in kornia-core-ops/src/ops.rs?
If so, how should we keep the ability to call for example tensor.powi(2) since we can't define the impl for a foreign type? Do we need a wrapper struct?

@edgarriba
Copy link
Member

possibly wiht a TensorOps trait but let's explore this later. Put a first an initial crate with something simple and we keep iterating

@jandremarais
Copy link
Contributor Author

Ok. Should I start with something simple that is not yet implemented or something that is already implement for Tensor?

@edgarriba
Copy link
Member

maybe the norm operator needed by cosine distance

@jandremarais
Copy link
Contributor Author

I started with the norm operator but then realised it needed the sum reduce operator. So I did its implementation first.

@jandremarais jandremarais marked this pull request as ready for review September 25, 2024 07:46
@jandremarais
Copy link
Contributor Author

Marked this as ready for review so that the new core-ops crate can be merged without being blocked by all the ops that need to be implemented. I you would rather want this to be a bigger PR with more ops added first, I can change the status to draft and work on it further.

crates/kornia-core-ops/src/ops.rs Outdated Show resolved Hide resolved
crates/kornia-core/src/tensor.rs Outdated Show resolved Hide resolved
crates/kornia-core-ops/src/error.rs Show resolved Hide resolved
crates/kornia-core-ops/src/ops.rs Show resolved Hide resolved
crates/kornia-core-ops/src/ops.rs Show resolved Hide resolved
@edgarriba
Copy link
Member

@jandremarais any progress here ? we can land step by step and keep improving

@jandremarais
Copy link
Contributor Author

I will work on this tonight (UTC +2). I will remove the dynamic shape stuff, add docs to sum_elements and put the DimOutOfBounds error in TensorError.

@edgarriba
Copy link
Member

@jandremarais sound good -- consider this too apache/arrow-rs#6492 (comment). I might be working on a PR for it during the week. Arrow seems pretty unflexible, so maybe we should go for our full implementation

@jandremarais
Copy link
Contributor Author

@edgarriba I addressed all of your comments in the latest commits. Open to further feedback if I interpreted anything incorrectly or if there are any new suggestions.

Copy link
Member

@edgarriba edgarriba left a comment

Choose a reason for hiding this comment

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

LGTM. Make a rebase and all good !

crates/kornia-core-ops/src/ops.rs Outdated Show resolved Hide resolved
@jandremarais jandremarais changed the title [wip] create new crate for core-ops create new crate for core-ops Oct 3, 2024
@edgarriba edgarriba merged commit fc17b65 into kornia:main Oct 3, 2024
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.

3 participants