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.

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

Fix/constant tensors #984

Merged
merged 11 commits into from
Nov 21, 2023
Merged

Fix/constant tensors #984

merged 11 commits into from
Nov 21, 2023

Conversation

nathanielsimard
Copy link
Member

@nathanielsimard nathanielsimard commented Nov 21, 2023 •
edited
Loading

In this effort to support constant tensors of any kind (Int and Bool, not just Float), I had to fix how we handle devices within modules.

Fix #461 #665 #982

antimora
antimora approved these changes Nov 21, 2023
View reviewed changes
Copy link
Collaborator

@antimora antimora left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. I had one comment. Please see below.

burn-core/src/module/param/tensor.rs
@@ -4,22 +4,36 @@ use crate::tensor::{
backend::{AutodiffBackend, Backend},
Tensor,
};
use alloc::vec::Vec;
use burn_tensor::{Bool, Int};

impl<B: Backend, const D: usize> From<Tensor<B, D>> for Param<Tensor<B, D>> {
fn from(value: Tensor<B, D>) -> Self {
Param::new(ParamId::new(), value.require_grad())
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to explain why we do not require grad?

Copy link
Member Author

@nathanielsimard nathanielsimard Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I added why we add require grad, other tensor kinds (bool and int) can't have gradients

dcvz
dcvz approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@dcvz dcvz left a comment

Choose a reason for hiding this comment

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

Nice work! 🔥 🚀

The devices() needing devices looks a bit odd to me. Especially when it sometimes leads being used as self.devices(Vec::new()) but I think I'm maybe missing something.

@antimora
Copy link
Collaborator

OK. We got a couple approvals and I need this PR for my onnx changes, hence I am merging.

@antimora antimora merged commit cabbaab into main Nov 21, 2023
7 checks passed
@nathanielsimard nathanielsimard deleted the fix/constant-tensors branch November 22, 2023 14:28
@nathanielsimard
Copy link
Member Author

Nice work! 🔥 🚀

The devices() needing devices looks a bit odd to me. Especially when it sometimes leads being used as self.devices(Vec::new()) but I think I'm maybe missing something.

Maybe we could rename the method, but we collect all the devices in the module tree in the provided vector. The alternative would imply a lot more allocations. Maybe we could rename the method to collect_devices and add a default implementation devices that uses collect_devices underneath.

@nathanielsimard nathanielsimard mentioned this pull request Nov 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcvz dcvz dcvz approved these changes

@antimora antimora antimora approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor and Simplify Device Handling
3 participants
@nathanielsimard @antimora @dcvz