Skip to content

Merge hcat into norm#14

Merged
SophieL1 merged 11 commits intosl/normfrom
sl/hcat
Oct 2, 2025
Merged

Merge hcat into norm#14
SophieL1 merged 11 commits intosl/normfrom
sl/hcat

Conversation

@SophieL1
Copy link
Collaborator

Goal is to add norm of a row vector and norm of a matrix

Notes: * Gradient is not correct yet (test fails)
* hcat cannot be evaluated as it is, since the final output must be scalar and we do not have any operation to reduce the matrix to scalar at the moment
0dim still to be fixed (hcat scalars into a row)
Meaning a row vector can now be written with [1 2] or hcat(1, 2)
To prevent checking if hcat has same size as children
ArrayDiff = "c45fa1ca-6901-44ac-ae5b-5513a4852d50"
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
MathOptInterface = "b8f27783-ece8-5eb3-8dc8-9495eed66fee"
Revise = "295af30f-e4ad-537b-8983-00126c2a3abe"
Copy link
Owner

Choose a reason for hiding this comment

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

The trick is: your environment is your global environment + your local one. So if you add Revise to your global environment, it will be available everywhere so no need to add it here :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for remark :) That wouldn't cause the tests to fail, would it?
The tests are passing on my side... I'll think some more.

Copy link
Owner

Choose a reason for hiding this comment

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

No, that's weird

@codecov
Copy link

codecov bot commented Oct 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (sl/norm@65fa6ef). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff             @@
##             sl/norm      #14   +/-   ##
==========================================
  Coverage           ?   99.22%           
==========================================
  Files              ?       10           
  Lines              ?     1550           
  Branches           ?        0           
==========================================
  Hits               ?     1538           
  Misses             ?       12           
  Partials           ?        0           

☔ 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.

* Remove Revise from toml as mentionned in review
* Remove hcat case 'more than 2 dims', as we do not pretend to handle it right now
# as long as sum or matx-vect products are not implemented.

#println("Last node ", f.nodes[1].index)
#if f.nodes[1].index == 12
Copy link
Owner

Choose a reason for hiding this comment

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

What is this if doing ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry it should have been deleted. It will be soon, thanks.
(I was checking that hcat vectors of length more than one was fine too. I think it was ok but was constructing a matrix and I had no further operation implemented to deal with it. I'll check this case after allowing norm of a matrix.)

end
# This function is written assuming that the final output is scalar.
# Therefore cannot return the matrix, so I guess I return it's first entry only,
# as long as sum or matx-vect products are not implemented.
Copy link
Owner

Choose a reason for hiding this comment

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

Good point, we should throw an error in this case, but let's do a separate PR for that

Copy link
Owner

Choose a reason for hiding this comment

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

#15

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. In principle though the functions we define take scalar values by definition, just as in the current version in ReverseAD.

@SophieL1
Copy link
Collaborator Author

SophieL1 commented Oct 2, 2025

If ok for you, we could merge this PR (especially since it's only merging hcat branch into norm branch; and this branch must still get some modifs before being merged to main).

@blegat
Copy link
Owner

blegat commented Oct 2, 2025

Oh I didn't see, feel free to merge. You can even merge into main since it's passing all the tests, up to you.

@SophieL1 SophieL1 merged commit da96225 into sl/norm Oct 2, 2025
5 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.

2 participants