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

autodiff: re-structure to avoid lifetimes. #72

Open
avhz opened this issue Jul 11, 2023 · 8 comments
Open

autodiff: re-structure to avoid lifetimes. #72

avhz opened this issue Jul 11, 2023 · 8 comments
Labels
difficult Hard or lengthy task. rework Re-work, re-structure, etc. Larger project.

Comments

@avhz
Copy link
Owner

avhz commented Jul 11, 2023

The Variable struct has a reference to the Graph struct. This is causing a lot of issues, namely it prevents:

  • Adding support for ndarray and nalgebra.
  • Python bindings since #[pyclass] is incompatible with lifetimes.

It also makes it somewhat non-user-friendly since the user also has to annotate functions they want to differentiate with lifetimes.

Need to find a better solution than &'v Graph (basically re-structure the module).

Edit:

Ideally, I want to implement num-traits for the Variable type, and allow it to be 'static, since both ndarray and nalgebra require array/matrix elements to be:

  • ndarray:

    • pub trait LinalgScalar: 'static + Copy + Zero + One + Add + Sub + Mul + Div<Output = Self> { }
  • nalgebra:

    • T: Scalar + Zero + ClosedAdd + ClosedMul
    • pub trait Scalar: 'static + Clone + PartialEq + Debug { }
@avhz avhz added rework Re-work, re-structure, etc. Larger project. difficult Hard or lengthy task. labels Jul 11, 2023
@SimonG85
Copy link
Contributor

The variable struct has a member Graph, maybe can be wrapped inside a Box?

@avhz
Copy link
Owner Author

avhz commented Jul 13, 2023

I tried Rc but it means user needs to .clone() a lot.

@laplus-sadness
Copy link

laplus-sadness commented Jul 13, 2023

Hello. I discovered this project due to the latest TWIR and was looking through this issue.

Can't Variable just own the Graph? Specially since you have a graph function that returns a reference to this member.

EDIT: sorry, I went trough the other files and realized this won't do. Boxing and using an rc would be fine since you are only cloning the pointer so there are no performance losses.

@avhz
Copy link
Owner Author

avhz commented Jul 13, 2023

Hi thanks for your interest :)

Rc works (after a lot of refactoring), but it's very annoying for the user.

Long mathematical formulas become filled with .clone() which is annoying to write and doesn't look very good.

I think the actual logic of the module needs to change tbh.

@laplus-sadness
Copy link

I am not familiar with automatic differentiation so I was reading the Wikipedia page about it and stumbled upon the cpp implementation of the reverse accumulation procedure, where they use a hashmap to store state.

So... I think that rewriting the Graph struct to own a vector, or a hashmap, of Variables, adding them through Graph, would do the things you are asking for.

@avhz
Copy link
Owner Author

avhz commented Jul 14, 2023

So store the Variables in the Vertexs ?

@Autoparallel
Copy link
Contributor

Has this been addressed?

@avhz
Copy link
Owner Author

avhz commented Feb 25, 2024

Not yet, it's quite a task. But it would be nice to get a more flexible AD implementation that can be used to compute Greeks more easily, for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficult Hard or lengthy task. rework Re-work, re-structure, etc. Larger project.
Projects
Status: No status
Development

No branches or pull requests

4 participants