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 MDNDensityEstimator #1042

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jnsbck
Copy link
Contributor

@jnsbck jnsbck commented Mar 19, 2024

…oned, and corrected for proposals, bot in SNPE-A and SNPE-C

What does this implement/fix? Explain your changes

This PR attempts to consolidate the functionalities of the various mixture density objects used throughout the toolbox, for details see #989 into one MixtureDensityEstimator. I will try to do this by implementing a MoG object on which conditioning, marginalization and proposal correction can be done if necessary, but it behaves like a Flow otherwise.

Does this close any currently open issues?

Fixes #989

Any relevant code examples, logs, error output, etc?

...

Any other comments?

PR is a WIP and subject to change. dev.ipynb already contains a working sketch of the implemetation though.

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.

  • I have read and understood the contribution
    guidelines
  • I agree with re-licensing my contribution from AGPLv3 to Apache-2.0.
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have reported how long the new tests run and potentially marked them
    with pytest.mark.slow.
  • New and existing unit tests pass locally with my changes
  • I performed linting and formatting as described in the contribution
    guidelines
  • I rebased on main (or there are no conflicts with main)

…oned, and corrected for proposals, bot in SNPE-A and SNPE-C
@jnsbck jnsbck force-pushed the 989-add-a-common-mdn-network-interface-consistent-with-new-densityestimators branch from 3ded687 to 7a4d504 Compare March 20, 2024 16:03
…oned, and corrected for proposals, bot in SNPE-A and SNPE-C
…oned, and corrected for proposals, bot in SNPE-A and SNPE-C
…new-densityestimators' of github.com:sbi-dev/sbi into 989-add-a-common-mdn-network-interface-consistent-with-new-densityestimators
@gmoss13 gmoss13 mentioned this pull request Mar 20, 2024
8 tasks
@janfb janfb added the blocked Something is in the way of fixing this. Refer to it in the issue label Apr 3, 2024
@janfb
Copy link
Contributor

janfb commented Apr 9, 2024

Hi @jnsbck, can you please add a description of this PR?
I am sure the PR makes total sense, I just could see directly what it is planning to do.

another comment: MixedDensityEstimator is already used in the context of mixed data (for MNLE). Maybe you can call it MixtureDensityEstimator?

Thanks 🙏

@jnsbck
Copy link
Contributor Author

jnsbck commented Apr 9, 2024

Yea of course. Was planning to continue working on it. Left the desc blank since its in draft state. Yes, can rename it to MixtureDensityEstimator. :)

EDIT: updated the desc. working on this is on my Todo :)

@janfb
Copy link
Contributor

janfb commented Apr 23, 2024

Ping @jnsbck :)
What is the status here, is there anything we can help with?

@jnsbck
Copy link
Contributor Author

jnsbck commented Apr 23, 2024

Have too much of other stuff on my plate currently, so I have not been actively working on this. Still on my todo though. I hope I will get around to finishing this at some point. Feel free to take the template and add to it though if you need some of the functionality urgently.

Will let you know when I start working on it again and where I need help :)

@janfb
Copy link
Contributor

janfb commented Apr 24, 2024

OK, thank you for the update @jnsbck 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something is in the way of fixing this. Refer to it in the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a common MDN network interface (consistent with new DensityEstimators)
2 participants