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

Added Jacobian Features in Calculus functions.py #39283

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Kritika75
Copy link

@Kritika75 Kritika75 commented Jan 5, 2025

Enhanced the jacobian function to include additional features: computation of Hessian matrices for single functions, optional sparse matrix representation for large systems, and the ability to calculate the rank of the Jacobian matrix.

Fixes #39218

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@vincentmacri
Copy link
Contributor

@tscrim (since I know you have the permission to approve workflows from first-time contributors) would you be willing to take a look at this? Or approve the CI workflows so I can review it?

Copy link

Documentation preview for this PR (built with commit ec51fc1; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@tscrim
Copy link
Collaborator

tscrim commented Jan 20, 2025

@vincentmacri It has been done by someone else. Please go ahead and do the review. I can re-approve workflows as needed.

@@ -147,4 +147,16 @@ def jacobian(functions, variables):
if not isinstance(variables, (tuple, list, Vector)):
variables = [variables]

return matrix([[diff(f, v) for v in variables] for f in functions])
jacobian_matrix([[diff(f, v) for v in variables] for f in functions])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant:

Suggested change
jacobian_matrix([[diff(f, v) for v in variables] for f in functions])
jacobian_matrix = matrix([[diff(f, v) for v in variables] for f in functions])

Copy link
Contributor

Choose a reason for hiding this comment

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

But with the other changes I suggested you should just revert the jacobian function back to how it was, and add a hessian function for that functionality.

@@ -106,7 +106,7 @@ def row(n):
return matrix([row(r) for r in range(len(fs))]).determinant()


def jacobian(functions, variables):
def jacobian(functions, variables, hessian=False, sparse=False, rank=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that hessian should be its own function unless you have a good reason for it not to be separate.

I don't think we need the rank option since the returns matrix already has a .rank() method that can be called on it.

@vincentmacri
Copy link
Contributor

vincentmacri commented Jan 20, 2025

I also don't see why the sparse option is needed when you could just call .sparse_matrix() on the returned matrix.

@vincentmacri
Copy link
Contributor

@Kritika75 In summary, unless there is some good reason I'm missing for why you implemented things the way you did, can you make these three changes?

  1. Remove the hessian option and add a hessian function instead.
  2. Remove the rank option, it's unnecessary since matrices already have a .rank() method. It also complicates type checking if the function can possibly return two different types (either a matrix or a tuple).
  3. Remove the sparse option, it's unnecessary since matrices already have a .sparse_matrix() method.

Since you're a first-time contributor, GitHub is kind of annoying about running the CI tests on your PR and someone needs to approve every test run manually. To speed things up, please test things locally and make sure your tests pass before requesting another review.

@tscrim
Copy link
Collaborator

tscrim commented Jan 21, 2025

I also don't see why the sparse option is needed when you could just call .sparse_matrix() on the returned matrix.

You don't want to create a dense matrix just to make it sparse. This is wasteful in both memory and computation time.

@tscrim
Copy link
Collaborator

tscrim commented Jan 21, 2025

That being said, the code itself should directly create thr sparse matrix, otherwise it should not have such an option as @vincentmacri said.

Although in nearly all cases, this will not be a sparse matrix. So there might not be much practicality in having it (and hence, the maintenance burden of such code).

@Kritika75
Copy link
Author

Apologies for the delay in getting back to you. I had my exams, but I completely understand your concern and will address it promptly

Copy link
Contributor

@vincentmacri vincentmacri left a comment

Choose a reason for hiding this comment

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

One small change request. Looks good otherwise!

src/sage/calculus/functions.py Outdated Show resolved Hide resolved
@vincentmacri
Copy link
Contributor

@Kritika75 also make sure to pull the latest changes from develop, otherwise git won't let us merge this.

@Kritika75 Kritika75 mentioned this pull request Feb 4, 2025
5 tasks
@vincentmacri
Copy link
Contributor

@Kritika75 you need to merge the changes from develop into your patch-1 branch.

@vincentmacri
Copy link
Contributor

If you need a quick git tutorial.

  1. Make sure the develop branch of your fork is up to date with the sage develop branch
  2. On your machine, from the develop branch run git pull. Then change to your patch-1 branch and run git merge develop. Then git push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend Jacobian Features in Calculus
3 participants