Skip to content

Hessian#43

Merged
Transurgeon merged 24 commits intomasterfrom
hessian
Sep 18, 2025
Merged

Hessian#43
Transurgeon merged 24 commits intomasterfrom
hessian

Conversation

@dance858
Copy link
Collaborator

Description

Please include a short summary of the change.
Issue link (if applicable):

Type of change

  • New feature (backwards compatible)
  • New feature (breaking API changes)
  • Bug fix
  • Other (Documentation, CI, ...)

Contribution checklist

  • Add our license to new files.
  • Check that your code adheres to our coding style.
  • Write unittests.
  • Run the unittests and check that they’re passing.
  • Run the benchmarks to make sure your change doesn’t introduce a regression.

Copy link
Member

@Transurgeon Transurgeon left a comment

Choose a reason for hiding this comment

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

really nice job @dance858 ! I left many comments, some are easy fixes, others we can discuss more. I will do another review of the tests at some point.

x = self.args[0]
y = self.args[1]
assert(x.size == y.size)

Copy link
Member

Choose a reason for hiding this comment

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

what if we have constant * constant? Maybe we should have a global hess_vec implementation in expression that checks for:

  1. If the expression is constant, return empty dict for hess
  2. If the expression is affine, same thing.

What do you think?

Copy link
Collaborator Author

@dance858 dance858 Sep 18, 2025

Choose a reason for hiding this comment

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

I totally agree! I added it to the atom class. Please take a look. I also added some error handling there. I like this abstraction.

Should we move it to expression? What do you think?

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 leaving it in atoms is the right place since that's where we had the hess implementation from before.

@Transurgeon Transurgeon merged commit 8f81cf0 into master Sep 18, 2025
0 of 24 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