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

feat: added kernel density estimation to score matching. #258

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

pc532627
Copy link
Contributor

Refs: 257

PR Type

  • Feature

Description

When converting kernel.py to OOP, we removed the functionality to (easily) estimate a score function via kernel density estimation. This has been added back into score_matching.py to provide an alternative to the neural network approach.

How Has This Been Tested?

Added test class TestKernelDensityMatching with:

Test A: test_univariate_gaussian_score
Test B: test_multivariate_gaussian_score
Test C: test_univariate_gmm_score
Test D: test_multivariate_gmm_score

Does this PR introduce a breaking change?

No

Checklist before requesting a review

  • I have made sure that my PR is not a duplicate.
  • My code follows the style guidelines of this project.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have performed a self-review of my code.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • Any dependent changes have been merged and published in downstream modules.

@pc532627 pc532627 requested a review from tl82649 November 28, 2023 11:06

return children, aux_data

def match(self, x: ArrayLike) -> Callable:
Copy link
Member

Choose a reason for hiding this comment

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

If x is not used, should we put x: ArrayLike | None = None? Then we don't need to call match with anything; particularly in test_score_matching.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed and added


# Extract the score function (this is not really learned from the data, more
# defined within the object)
learned_score = kernel_density_matcher.match(samples)
Copy link
Member

Choose a reason for hiding this comment

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

If comment here is followed, we shouldn't need the match argument.

Repeat for all other calls to match in TestKernelDensityMatching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to not input anything (since it's unused)

Copy link
Member

@tl82649 tl82649 left a comment

Choose a reason for hiding this comment

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

All looks fine functionally. I suggest making the x argument to match optional in KernelDensityMatching, then remove the arguments accordingly in test_score_matching.py. See comments for detail.

@pc532627 pc532627 requested a review from tl82649 December 5, 2023 14:34
@pc532627 pc532627 merged commit 7d587e1 into feature/oop_106 Dec 6, 2023
1 of 8 checks passed
@pc532627 pc532627 deleted the feature/kde_for_score_matching_257 branch December 6, 2023 12:13
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