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
Enhance posterior viz #58
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a few comments. In general, I think I'd prefer a less "thin" wrapper, i.e. make the API more like brian2modelfitting
(use dictionaries with parameter names and quantities) than sbi
(use PyTorch tensors). Also, maybe add example calls for these functions somewhere, e.g. in the advanced sbi example?
But if this PR keeps you from advancing, feel free to merge it as it is and we can deal with these details later.
brian2modelfitting/inferencer.py
Outdated
try: | ||
d = self.posterior | ||
except AttributeError as e: | ||
print(e, '\nDensity is not available.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should rather be something like
raise AttributeError('Density is not available.') from e
instead of print
+ raise
brian2modelfitting/inferencer.py
Outdated
|
||
Parameters | ||
---------- | ||
condition : torch.tensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the rest of the code we tried to avoid that the user has to deal with pytorch tensors themselves. Maybe this could be something like a dictionary mapping parameter names to values (like the parameters in generate
, for example)? But then this should be probably what sample
returns as well...
brian2modelfitting/inferencer.py
Outdated
condition : torch.tensor | ||
Condition that all but the one/two regarded parameters are | ||
fixed to. | ||
limits : list or torch.tensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we make this optional and reuse the bounds here?
brian2modelfitting/inferencer.py
Outdated
condition : torch.tensor | ||
Condition that all but the one/two regarded parameters are | ||
fixed to. | ||
limits : list or torch.tensor | ||
Limits in between which each parameter will be evaulated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remarks as above.
All your comments are taken into account. As we discuss, I will merge this to |
Resolves #51
Added method
conditional_pairplot
andconditional_corrcoeff
toInferencer
class, which are basically thin wrappers aroundsbi.analysis.plot.conditional_pairplot
andsbi.analysis.conditional_corrcoeff
, respectively.Also features are now optional argument, if
features
are not provided, the density estimator will employ a MLP by default in order to automatically extract features from high-D data.