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

Do not throw an Exception when the effect is not identifiable #2

Open
carloscinelli opened this issue Aug 3, 2021 · 2 comments
Open

Comments

@carloscinelli
Copy link

carloscinelli commented Aug 3, 2021

Martí,

Congratulations on this very simple to use package, it seems great so far.

One design suggestion though.

Instead of throwing an Exception when the effect is not identifiable, I believe you should still return a probability object, but with exactly the information you now provide in the Exception message: that the effect is not ID and the hedge.

The reason being that the effect not being identifiable is not an error in itself, it is the correct behavior of the algorithm.

So it is better to return the formal result of the algorithm (non-identifiability and the reason for non-identifiability) in a way that can be stored in an object for further manipulations.

An exception should be used for real errors and exceptions only.

@carloscinelli carloscinelli changed the title Do not throw an Exception when the effect is not identification Do not throw an Exception when the effect is not identifiable Aug 3, 2021
@pedemonte96
Copy link
Owner

Dear Carlos,

First of all, thank you for your kind words. This has been a tricky project, and I am pleased my contribution can help the scientific community.

Regarding your suggestion, how would you implement this modification? The fact that the causal effect is not identifiable cannot be encoded in a probability object, because it is not a probabiliy. The thrown exception has to be interpreted as a natural and possible outcome of the algorithm, and not an error.

I understand your concern, but I do not know how to encode the information of non-identifiability into the Probability class.

@carloscinelli
Copy link
Author

carloscinelli commented Aug 15, 2021

Hi Marti,

I understand it is not a probability, but it is also not an error.

The way it is currently designed, we would need to handle the exception to capture the useful information you provide (i.e, that the effect is not ID, the hedge etc). So I believe a better solution would be that the function instead returns an output with that information.

(Tikka also made this choice of producing an error in his first causal.effect package, but notice he doesn't do that anymore in the newer packages.)

But in the end of the day, of course, this is a design choice! Just a suggestion.

Best, Carlos

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