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

bat_marginalize should come with an algorithm type and defaults #132

Open
oschulz opened this issue Jun 9, 2020 · 7 comments
Open

bat_marginalize should come with an algorithm type and defaults #132

oschulz opened this issue Jun 9, 2020 · 7 comments
Assignees

Comments

@oschulz
Copy link
Member

oschulz commented Jun 9, 2020

bat_marginalize should always return a NamedTuple (result = ...,), to leave room for additional, algorithm-specific output values. This is something like a standard for all bat_....() functions now. Sorry, forgot to mention that on the PR that added bat_marginalize.

CC @Cornelius-G, @VasylHafych

@Cornelius-G
Copy link
Collaborator

I guess this issue can also be closed as it was fixed with PR #133

@oschulz
Copy link
Member Author

oschulz commented Jun 23, 2020

Oh, yes. But we have the more rigorous API for bat_functions now, so I'll change the title of this issue. :-)

@oschulz oschulz changed the title bat_marginalize should return a NamedTuple bat_marginalize should come with an algorithm type and defaults Jun 23, 2020
@oschulz
Copy link
Member Author

oschulz commented Jun 23, 2020

So we should add the concept of marginalization to algotypes and algodefaults.

@Cornelius-G
Copy link
Collaborator

Ah, ok. I will take a look at this.
We also wanted to think about the general structure of the marginalizations again.
So currently they have the original valueshape and the indices of the included parameters. But I guess we wanted to have them only the valueshape of the contained params, right?

@oschulz
Copy link
Member Author

oschulz commented Jun 23, 2020

Hm, from a purist point of view, they would not include the indices of the original parameters. Do we need them?

Actually, I've been thinking about adding a close relative to ValueShapes.jl that allows assigning a varshape to any generic multivariate Distribution. With that, we wouldn't even need a special data structure for marginalizations in BAT itself, it would just be a combinations of ValueShapes and EmpiricalDistributions.

@Cornelius-G
Copy link
Collaborator

Ah, what should we now do about this issue?

Hm, from a purist point of view, they would not include the indices of the original parameters. Do we need them?

I will check again if we need the original shapes for a correct matching of names to indices in plotting, but I think we actually would not need them for this.
If not needed, should we then make MarginalDist contain only the distribution and a subset of the original shape?

Or will you take a look into this proposed

close relative to ValueShapes.jl that allows assigning a varshape to any generic multivariate Distribution.

?

@oschulz
Copy link
Member Author

oschulz commented Jul 30, 2020

think we actually would not need them for this.

Then let's drop them, it'll be much cleaner without.

If not needed, should we then make MarginalDist contain only the distribution and a subset of the original shape

Yes - I'm thinking about adding a generic "add NamedTupleShape to any dist" wrapper to ValueShapes - that would then make it possible to replace MarginalDist by that wrapper combined EmpiricalDistributions.

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

2 participants