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

Add better interaction with and export to sympy, and uses sympy to implement more Hessian functions #191

Merged
merged 18 commits into from Oct 15, 2020

Conversation

adrn
Copy link
Owner

@adrn adrn commented Oct 13, 2020

Describe your changes

This adds a .to_sympy() classmethod to the potential classes. I've also then used this method with sympy to compute all of the Hessians, and implemented these using C code generated by sympy.

Checklist

  • Did you add tests?
  • Did you add documentation for your changes?
  • Did you add a changelog entry? (see CHANGES.rst)
  • Are the CI tests passing?
  • Is the milestone set?

Amazingly, this closes #159, closes #56, closes #5, and closes #85 !!

@adrn adrn added this to the v1.3 milestone Oct 13, 2020
@adrn
Copy link
Owner Author

adrn commented Oct 14, 2020

cc @smoh - this finally fixes some of the Hessian issues you were having (...last year 😶)

@adrn adrn changed the title Add better interaction with and export to sympy Add better interaction with and export to sympy, and uses sympy to implement more Hessian functions Oct 14, 2020
@adrn
Copy link
Owner Author

adrn commented Oct 14, 2020

Before merging, make an issue to remind me to figure out how to implement to_sympy() for SCFPotential (EDIT: done in #193)

@adrn adrn marked this pull request as ready for review October 14, 2020 20:05
@smoh
Copy link
Contributor

smoh commented Oct 14, 2020

cc @smoh - this finally fixes some of the Hessian issues you were having (...last year 😶)

Cool! Yea, I never got around to that.. That's a lot of tmps! 😅 Wouldn't it be neat if we could generate gradient funcs from sympy expression!

@adrn
Copy link
Owner Author

adrn commented Oct 15, 2020

That's a lot of tmps! 😅

Heh, yea...at least they were auto-generated! 😄

Wouldn't it be neat if we could generate gradient funcs from sympy expression!

Do you mean in the same way that I generated the hessians, or you want to be able to create a potential class from an arbitrary sympy expression?

@smoh
Copy link
Contributor

smoh commented Oct 15, 2020

Oh, cool -- I missed this comment

/* Note: many Hessians generated with sympy in
    gala-notebooks/Make-all-Hessians.ipynb
*/

If you are autogenerating the C code from sympy, I guess you are already kind of doing it! Looking at it again, probably no sane person can stay on top of doing that by hand..!

And I see that you are comparing to the function generated using sympy's lambdify. I meant something like an integration with sympy's lambdify so that people can define e.g., potential by a symbolic expression, and any gradients would follow from sympy, too.

I'm actually curious how it was done in the notebook, which seems to be not available anywhere, specifically how you figured out the common factors to store in tmp variables. Is it a functionality sympy offers?

That's a lot of tmps! 😅

Heh, yea...at least they were auto-generated! 😄

Wouldn't it be neat if we could generate gradient funcs from sympy expression!

Do you mean in the same way that I generated the hessians, or you want to be able to create a potential class from an arbitrary sympy expression?

@adrn
Copy link
Owner Author

adrn commented Oct 15, 2020

And I see that you are comparing to the function generated using sympy's lambdify. I meant something like an integration with sympy's lambdify so that people can define e.g., potential by a symbolic expression, and any gradients would follow from sympy, too.

Oh yea, that's a good idea! It would be a lot slower (unless I can figure out a way to combine with numba) but that would be cool. There's some limited support for this already, actually, but it could use some cleaning! https://gala-astro.readthedocs.io/en/latest/api/gala.potential.potential.from_equation.html#gala.potential.potential.from_equation

I'm actually curious how it was done in the notebook, which seems to be not available anywhere, specifically how you figured out the common factors to store in tmp variables. Is it a functionality sympy offers?

It's a bit of a mess, but here it is: https://gist.github.com/e1aa71fbb11167d6109fb7077020d630

@adrn adrn merged commit b284bf8 into main Oct 15, 2020
@smoh
Copy link
Contributor

smoh commented Oct 15, 2020

Thanks! That's pretty cool.

@adrn adrn deleted the to-sympy-meth branch October 15, 2020 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants