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.

Sign up for GitHub

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

Jump to bottom

Sprint14 fd gradient #142

Merged
merged 44 commits into from
Jan 31, 2023
Merged

Sprint14 fd gradient #142

merged 44 commits into from
Jan 31, 2023

Conversation

amal-ghamdi
Copy link
Contributor

@amal-ghamdi amal-ghamdi commented Nov 4, 2022 •
edited
Loading

closes #143

@amal-ghamdi amal-ghamdi mentioned this pull request Nov 4, 2022
Closed
5 tasks
@amal-ghamdi amal-ghamdi marked this pull request as ready for review December 5, 2022 23:05
nabriis
nabriis requested changes Dec 7, 2022
View reviewed changes
Copy link
Collaborator

@nabriis nabriis left a comment

Choose a reason for hiding this comment

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

We discussed the suggested changes in a meeting. Basic summary is to make the FD array type independant, and removing the init making it into a configure_FD method instead.

@amal-ghamdi
Copy link
Contributor Author

We discussed the suggested changes in a meeting. Basic summary is to make the FD array type independant, and removing the init making it into a configure_FD method instead.

Thanks @nabriis for your feedback. I made the updates we talked about along with using FD method (utilities.approx_gradient) that is specific for scalar functions (since logd is a scalar valued function). It will be great if you can take a look.

nabriis
nabriis approved these changes Jan 13, 2023
View reviewed changes
Copy link
Collaborator

@nabriis nabriis left a comment

Choose a reason for hiding this comment

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

Hi Amal. This looks good to me now. I have two minor suggestions you can look at. Otherwise it looks ready.

cuqi/utilities/_utilities.py Outdated Show resolved Hide resolved
cuqi/utilities/_utilities.py Outdated Show resolved Hide resolved
jakobsj
jakobsj requested changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@jakobsj jakobsj left a comment

Choose a reason for hiding this comment

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

Thank you for this comprehensive PR adding finite different based gradient. I find it is in great shape and have added some minor comments and questions. This issue it is aimed at, namely #143 mentions another issue to be addressed #157, is this covered by this PR or for another PR?

cuqi/density/_density.py Outdated Show resolved Hide resolved
cuqi/distribution/_distribution.py Outdated Show resolved Hide resolved
cuqi/likelihood/_likelihood.py Outdated Show resolved Hide resolved
cuqi/samples/_samples.py Show resolved Hide resolved
cuqi/utilities/_utilities.py Outdated Show resolved Hide resolved
tests/test_likelihood.py Show resolved Hide resolved
cuqi/utilities/_utilities.py Show resolved Hide resolved
cuqi/utilities/_utilities.py Outdated Show resolved Hide resolved
cuqi/utilities/_utilities.py Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
@amal-ghamdi
Copy link
Contributor Author

Thank you @jakobsj for the very helpful review and great suggestions! Issue #157 was created to handle this bug separately. It is not fixed as part of this PR.

@amal-ghamdi
Copy link
Contributor Author

Thank you @jakobsj for your review! I updated the PR reflecting your comments and suggestions. I appreciate it if you can take a look.

jakobsj
jakobsj approved these changes Jan 31, 2023
View reviewed changes
Copy link
Contributor

@jakobsj jakobsj left a comment

Choose a reason for hiding this comment

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

Everything addressed nicely, thanks!

@amal-ghamdi amal-ghamdi merged commit b7ee795 into main Jan 31, 2023
@amal-ghamdi amal-ghamdi deleted the sprint14_FD_gradient branch January 31, 2023 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nabriis nabriis nabriis approved these changes

@jakobsj jakobsj jakobsj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Discussion: Enable FD approximation for posterior gradient (density in general)
3 participants
@amal-ghamdi @jakobsj @nabriis