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

Extension operands_and_partials to 8 edges #2698

Closed
2 tasks done
Franzi2114 opened this issue Apr 6, 2022 · 4 comments
Closed
2 tasks done

Extension operands_and_partials to 8 edges #2698

Franzi2114 opened this issue Apr 6, 2022 · 4 comments

Comments

@Franzi2114
Copy link
Collaborator

Franzi2114 commented Apr 6, 2022

Description

As I want to add a function with 8 known partials to the stan-math library (#2682), I had to extend the operands_and_partials routine such that it can work with 8 instead of 5 partial derivatives.

Example

It is possible to fill up to 8 edges for operands_and_partias:
ops_partials.edge8_.partials= ...

Questions

  • Do I have to set up an own Pull Request for this? Or can it go together with the Pull Request for the new function?
  • Which tests are required for this purpose before I ask can for the Pull Request?

Current Version:

v4.3.2

@bob-carpenter
Copy link
Contributor

can it go together with the Pull Request for the new function?

Yes.

Which tests are required

Value tests and then derivative tests for the function. I'm not sure how operands_and_partials is tested itself, but you can follow whatever tests are already there.

@SteveBronder
Copy link
Collaborator

I think at the point we have 8 edges it might be good to turn the impl into having an underlying std::tuple just holding all the edges we need

@Franzi2114
Copy link
Collaborator Author

I think at the point we have 8 edges it might be good to turn the impl into having an underlying std::tuple

Yeah, maybe that makes sense. But I would suggest that somebody does it at a later point of time since right now, the new 7-parameter function works with those few changes I did. And @bob-carpenter agreed to bring these changes in the same Pull Request as the new 7-parameter function. The change of the underlying structure sounds a bit more complex.

@Franzi2114
Copy link
Collaborator Author

This was done in PR #2833.

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

No branches or pull requests

3 participants