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

kernel/ksupport: add linalg functions #2527

Open
wants to merge 1 commit into
base: nac3
Choose a base branch
from

Conversation

AR-PyT
Copy link
Contributor

@AR-PyT AR-PyT commented Aug 1, 2024

ARTIQ Pull Request

Description of Changes

Adds runtime implementation of np.linalg/sp.linalg functions (M-Labs/nac3#478 and M-Labs/artiq-zynq#309)
Depends on #2526

Related Issue

Type of Changes

Type
✨ New feature

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

  • Use correct spelling and grammar.

Code Changes

  • Run flake8 to check code style (follow PEP-8 style). flake8 has issues with parsing Migen/gateware code, ignore as necessary.

Licensing

See copyright & licensing for more info.
ARTIQ files that do not contain a license header are copyrighted by M-Labs Limited and are licensed under LGPLv3+.

@@ -117,6 +117,19 @@ static mut API: &'static [(&'static str, *const ())] = &[
api!(y1),
api!(yn),

// linalg
api!(np_linalg_matmul = ::linalg::np_linalg_matmul),
Copy link
Member

Choose a reason for hiding this comment

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

Again I don't think a firmware call and Rust crate are necessary for a couple for loops with a few additions and multiplications in them.

Copy link
Member

Choose a reason for hiding this comment

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

Also this is already implemented via the @ operator so AFAICT you can just remove this entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this is already implemented via the @ operator so AFAICT you can just remove this entirely.

Should I also remove np_linalg_det as that too can be implemented in a straight-forward manner.

Copy link
Member

Choose a reason for hiding this comment

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

I think determinant isn't that straightforward and there are a bunch of different algorithm? Maybe using the same crate that does the other operations is fine here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@AR-PyT AR-PyT force-pushed the add_linalg_functions branch from 0795195 to 4e37ac4 Compare August 1, 2024 09:51
@sbourdeauducq
Copy link
Member

Is this tested? I don't think this is going to work without adding nalgebra to cargo.
If this is hassle because Rust, we can also skip support for nalgebra on risc-v devices, since it's probably quite slow there anyway.

@sbourdeauducq sbourdeauducq added this to the ARTIQ-9 milestone Aug 1, 2024
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