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

Unilateral risk does not provide risk array anymore #87

Closed
YoelPH opened this issue Jun 24, 2024 · 4 comments
Closed

Unilateral risk does not provide risk array anymore #87

YoelPH opened this issue Jun 24, 2024 · 4 comments
Assignees
Labels
code quality Improvements w.r.t. readability of code & best practices of coding

Comments

@YoelPH
Copy link
Contributor

YoelPH commented Jun 24, 2024

the risk function in Unilateral does not provide an array with the risk for each state anymore.
Even though lines 884-887 state, that only if we specify an involvement we marginalize, there is no if statement to do so:

        # if a specific involvement of interest is provided, marginalize the
        # resulting vector of hidden states to match that involvement of
        # interest
        return self.marginalize(involvement, posterior_state_dist)

I am not sure whether this is intended. However I prefer something like:

        if involvement != None:
            return self.marginalize(involvement, posterior_state_dist)
        return posterior_state_dist

The risk array can be quite useful for alternative analysis of the output.

@rmnldwg
Copy link
Owner

rmnldwg commented Jun 24, 2024

You can use the posterior_state_dist() method for that.

I changed it because in my opinion a risk is simply the posterior state distribution marginalized over the matching patterns of involvement. That's why the risk() method now simply calls the posterior_state_dist() and then the marginalize() method.

Previously, when calling the risk() with involvement=None, you would not get a risk, but the posterior state distribution. SO, I was hoping this is more semantic 😅

Does that work for you?

@YoelPH
Copy link
Contributor Author

YoelPH commented Jun 24, 2024

Sounds good!
Then the output when no involvement is given should maybe give some information as well why it is just 1? Because running the risk without involvement does not make sense in this case.

@rmnldwg
Copy link
Owner

rmnldwg commented Jun 24, 2024

Hm yes, maybe... My thinking was that we typically provide None to the involvement pattern when we are not interested in a particular LNL's involvement. So, when someone provides None as the entire involvement pattern, then we just marginalize over everything 😅

But yeah, it is somewhat pointless here. We could just make the involvement argument required, not allowing it to be None.

@rmnldwg rmnldwg added code quality Improvements w.r.t. readability of code & best practices of coding labels Jun 24, 2024
@YoelPH
Copy link
Contributor Author

YoelPH commented Jun 24, 2024

Exactly. If you know about it, there is no problem. But if one sees that involvement is not required, one might be lost why there is always just 1 as output.

@rmnldwg rmnldwg self-assigned this Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Improvements w.r.t. readability of code & best practices of coding
Projects
None yet
Development

No branches or pull requests

2 participants