Add burn::linalg::{vector_norm,l2_norm} #3131

crutcher · 2025-05-01T05:13:05Z

Pull Request Template

Checklist

Confirmed that run-checks all script has been executed.
Made sure the book is up to date with changes in this PR.

Changes

Added burn::linalg::{vector_norm,l2_norm}

Testing

Testgen tests.

codecov · 2025-05-01T06:10:21Z

Codecov Report

Attention: Patch coverage is 94.65021% with 13 lines in your changes missing coverage. Please review.

Project coverage is 81.47%. Comparing base (f77b405) to head (8622f98).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...rates/burn-tensor/src/tensor/linalg/vector_norm.rs	84.70%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3131      +/-   ##
==========================================
+ Coverage   81.39%   81.47%   +0.08%     
==========================================
  Files         821      823       +2     
  Lines      118058   118301     +243     
==========================================
+ Hits        96088    96385     +297     
+ Misses      21970    21916      -54

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

laggui

That's a nice addition! Good start to the linalg module 🙂

A couple minor comments below.

Also, not sure about all the redefined functions for typical norms like l0_norm, l1_norm, etc. We could probably remove those and simply add the definitions to the docstring 🤔

crates/burn-tensor/src/tensor/linalg/vector_norm.rs

crates/burn-tensor/src/tests/linalg/vector_norm.rs

crutcher · 2025-05-01T18:36:44Z

I think there's a lot of value in having common functions defined explicitly, even if they're defined in terms of other functions. I could see renaming the +inf/-inf norm functions; if you have better names.

We could also fix the lack of .abs_min_dim(); and drop these; idk.

But if you think about these in terms of "how readable are callers", a lot of literature is written wrt l1 and l2 norms; and i think at least those should have their own alias functions.

laggui

But if you think about these in terms of "how readable are callers", a lot of literature is written wrt l1 and l2 norms; and i think at least those should have their own alias functions.

For readability and explicitness, I agree. But the negative norm names are a bit awkward indeed, and the match on the f64 as well.

What do you think about an enum?

Could be something along the lines of..

pub enum Norm {
    L0,
    L1,
    L2,
    LInf,
    LNegInf,
    Lp(f64),
}

For ease of use, we could accept p: N where N: Into<Norm>.

So one could specify Norm::L0 and other explicit norms. But also if we have From implementations we could have something more seamless. Just a quick example:

impl From<i32> for Norm {
    fn from(value: i32) -> Self {
        match value {
            0 => Norm::L0,
            1 => Norm::L1,
            2 => Norm::L2,
            v => Norm::Lp(v as f64)
        }
    }
}

could probably be added for i64, f32 and f64 for better type inference.

Lmk what you think 🙂

Otherwise, the actual implementation LGTM. Just a matter of defining the correct interface.

crates/burn-tensor/src/tensor/linalg/vector_norm.rs

ArthurBrussee · 2025-05-02T17:12:46Z

Drive-by comment: is it worth specializing L2 as diff * diff instead of a powf_scalar? Just since L2 is so common everywhere in ML, might be worth benching

crutcher · 2025-05-02T18:30:34Z

@ArthurBrussee I have specialized it (for the .powi_scalar(2) part); but I can't specialize for the .sqrt() call, and have to use the .powf_scalar(0.5) call; untill/unless I do some additional work on the trait bounds.

@laggui I ran with your idea; and updated the PR.

I would like someone to look at this and suggest what numeric traits, if any, we should bind the methods to; and how I express those traits.

It seems like some of these (max abs, min abs, count) don't need float; but l2 and lp both probably need float. (Also, it's ... weird that .powf_scalar() doesn't require the Float trait, but .sqrt() does ...)

crutcher · 2025-05-02T19:50:14Z

Please re-review.

I think I've addressed the type bounds to the best of my ability.

crutcher · 2025-05-02T20:25:10Z

Discovered, worked-around #3139

crutcher · 2025-05-02T21:01:03Z

Wait on: #3140

crutcher · 2025-05-03T07:10:05Z

Converted into a draft until the above bug / fix goes in.

crutcher · 2025-05-05T05:26:58Z

Should this be moved to burn-core?

laggui · 2025-05-05T12:18:49Z

Should this be moved to burn-core?

I briefly had a thought about this over the weekend. Most of the "tensor ops" are just being thrown in burn-tensor but I think we should have a better separation overall (base / numeric / etc ops are not always clearly distinguishable).

Right now burn-tensor probably still makes the most sense. In the future, if we have enough ops we could have a burn-linalg crate (re-exported via a feature flag).

crutcher · 2025-05-05T18:28:20Z

Done!
I've updated this now that the .max_dim() fix is in.

laggui

Just a minor comment regarding naming for the max/min norms to make sure.

Otherwise should be good to go! Thanks for going through the rounds in this PR.

laggui · 2025-05-06T12:38:59Z

crates/burn-tensor/src/tensor/linalg/vector_norm.rs

+/// # Returns
+///
+/// The L:INFINITY norm of the input tensor.
+pub fn max_abs_norm<B: Backend, const D: usize, K>(


Much better naming than l_inf_norm 😅

Is it called the max abs norm or just max norm typically? My linear algebra terminology is a little dusty.

Not really, it really is called the l-infinity-norm:
https://mathworld.wolfram.com/L-Infinity-Norm.html

Yes of course but there are often other names used interchangeably (e.g., euclidean norm or l2 norm).

Max norm seems to also be used, but not sure if it is that common. Anyway, either max_abs_norm, max_norm or infinity_norm (without the L prefix) should be suitable.

laggui · 2025-05-06T12:40:36Z

crates/burn-tensor/src/tests/linalg/vector_norm.rs

+        linalg::max_abs_norm(x.clone(), 0)
+            .into_data()
+            .assert_eq(&TestTensor::<2>::from([[3., 4.]]).into_data(), true);


Technically this would test pretty much the same code path, but I have no strong opposition against the additional test here.

Yes; it's verifying that the dispatch in vector_norm works

crutcher · 2025-05-06T17:03:20Z

I missed the previous discussion about the ops; I'll try and adjust

crutcher

will adjust

crutcher · 2025-05-06T16:59:21Z

crates/burn-tensor/src/tests/linalg/vector_norm.rs

+        linalg::max_abs_norm(x.clone(), 0)
+            .into_data()
+            .assert_eq(&TestTensor::<2>::from([[3., 4.]]).into_data(), true);


Yes; it's verifying that the dispatch in vector_norm works

Push a change to jiggle stuck test Action.

* Add burn::linalg::{vector_norm,l2_norm} * fmt * doc * freshen up inf * l1 norm * aliases * docs * close for l2 * approx_eq * long * improve tolerance * review * vector_normalize * Refactor for review * typing; make .sqrt() work for l2_norm * eps type conversion * test integer tensors, workaround new bug: tracel-ai#3139 * generalize * remove empty where clause * Remove workaround for tracel-ai#3119 * Update vector_norm.rs Push a change to jiggle stuck test Action.

laggui reviewed May 1, 2025

View reviewed changes

crates/burn-tensor/src/tensor/linalg/vector_norm.rs Outdated Show resolved Hide resolved

crates/burn-tensor/src/tensor/linalg/vector_norm.rs Outdated Show resolved Hide resolved

crates/burn-tensor/src/tests/linalg/vector_norm.rs Outdated Show resolved Hide resolved

crutcher requested a review from laggui May 1, 2025 18:36

laggui reviewed May 1, 2025

View reviewed changes

crates/burn-tensor/src/tensor/linalg/vector_norm.rs Outdated Show resolved Hide resolved

crutcher requested a review from laggui May 2, 2025 18:30

crutcher marked this pull request as draft May 3, 2025 03:07

crutcher force-pushed the crutcher/vector_norm branch from 286e080 to 94b8841 Compare May 5, 2025 18:27

crutcher marked this pull request as ready for review May 5, 2025 18:27

laggui reviewed May 6, 2025

View reviewed changes

crutcher commented May 6, 2025

View reviewed changes

crutcher added 9 commits May 6, 2025 14:37

Add burn::linalg::{vector_norm,l2_norm}

07b3ef8

fmt

8092f63

doc

fa64bf3

freshen up inf

593c8a0

l1 norm

799620a

aliases

f51b5cd

docs

753667b

close for l2

139788b

approx_eq

aa11290

crutcher added 12 commits May 6, 2025 14:37

long

596e1a7

improve tolerance

159e975

review

e0ba924

vector_normalize

7db3d57

Refactor for review

f09a8f1

typing; make .sqrt() work for l2_norm

d4d2cfe

eps type conversion

9474a6a

test integer tensors, workaround new bug: tracel-ai#3139

b94ab10

generalize

1271c18

remove empty where clause

86cf09f

Remove workaround for tracel-ai#3119

20318f4

Update vector_norm.rs

8622f98

Push a change to jiggle stuck test Action.

crutcher force-pushed the crutcher/vector_norm branch from b0e765e to 8622f98 Compare May 6, 2025 21:38

laggui approved these changes May 7, 2025

View reviewed changes

laggui merged commit 80de1fa into tracel-ai:main May 7, 2025
10 of 11 checks passed

antimora mentioned this pull request May 9, 2025

Linear algebra operations #1538

Open

crutcher deleted the crutcher/vector_norm branch May 20, 2025 17:15

Add burn::linalg::{vector_norm,l2_norm} #3131

Add burn::linalg::{vector_norm,l2_norm} #3131

Uh oh!

Conversation

crutcher commented May 1, 2025

Pull Request Template

Checklist

Changes

Testing

Uh oh!

codecov bot commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

laggui left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crutcher commented May 1, 2025

Uh oh!

laggui left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ArthurBrussee commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crutcher commented May 2, 2025

Uh oh!

crutcher commented May 2, 2025

Uh oh!

crutcher commented May 2, 2025

Uh oh!

crutcher commented May 2, 2025

Uh oh!

crutcher commented May 3, 2025

Uh oh!

crutcher commented May 5, 2025

Uh oh!

laggui commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crutcher commented May 5, 2025

Uh oh!

laggui left a comment

Choose a reason for hiding this comment

Uh oh!

laggui May 6, 2025

Choose a reason for hiding this comment

Uh oh!

crutcher May 6, 2025

Choose a reason for hiding this comment

Uh oh!

laggui May 6, 2025

Choose a reason for hiding this comment

Uh oh!

laggui May 6, 2025

Choose a reason for hiding this comment

Uh oh!

crutcher May 6, 2025

Choose a reason for hiding this comment

Uh oh!

crutcher commented May 6, 2025

Uh oh!

crutcher left a comment

Choose a reason for hiding this comment

Uh oh!

crutcher May 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 1, 2025 •

edited

Loading

laggui left a comment •

edited

Loading

ArthurBrussee commented May 2, 2025 •

edited

Loading

laggui commented May 5, 2025 •

edited

Loading