-
Notifications
You must be signed in to change notification settings - Fork 155
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
Use newest version of pyright, ignored a lot of errors, identified some bugs #1045
Conversation
…anageable number of errors
…y's rv_frozen/multi_rv_frozen @janfb
…wnstream arguments (reportArgumentType)
…n return type of Parallel generator (reportAssignmentType)
…know that there will be only one
…otlib has those propertoes
… suspect that they are compatible because they're all based on nn.Modules
This reverts commit 02b0c7e.
Thanks @Baschdl, great work! Some comments:
These functions only apply to mixture density networks - and should only be called when the density estimator is an MDN. Our implementation of MDN is using
Yes - this should resolved by updating the MixedDensityEstimator to the new DensityEstimator interface (see #968) - so I suggest we wait for #968 to be resolved before merging this @coschroeder . |
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.
Infinite karma for this! A few requests below, but feel free to merge then.
python_version = "3.8" | ||
reportUnsupportedDunderAll = false | ||
reportGeneralTypeIssues = false | ||
reportInvalidTypeForm = false |
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.
+1 for this, thank you!
@@ -101,60 +99,6 @@ def variance(self): | |||
) | |||
|
|||
|
|||
class ScipyPytorchWrapper(Distribution): |
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.
why remove this?
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.
@janfb's assessment was that no one is using this wrapper, so instead of trying to fix this issue he suggested to deprecate it.
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'll merge it for now but we can still bring it back. Let me know.
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.
great effort! 👏
Karma +1000
I worked through the errors pyright raises on the newest version. I used very small commits to be able to cherry pick if we only want to go with some of the decisions I made. You can rerun stuff with
pyright
locally (you can also find the errors in the CI run).I believe that the remaining errors are actual errors for which we don't have unit tests currently (I tagged the people who seem to be responsible for those parts of the code):
analysis/conditional_density.py
:sbi/sbi/analysis/conditional_density.py
Line 209 in 1d25046
Here you assume that the network of your new
DensityEstimator
has the same interface asFlow
. As an example, I would assume that the generalnn.Module
doesn't necessarily has the_distribution
attribute we access later on:sbi/sbi/utils/conditional_density_utils.py
Line 179 in 1d25046
neural_nets/mnle.py
:sbi/sbi/neural_nets/mnle.py
Line 96 in c347d5c
This error currently only says that the
__init__
ofMixedDensityEstimator
doesn't expect aNFlowsFlow
:sbi/sbi/neural_nets/mnle.py
Line 217 in c347d5c
The obvious thing would be to add it as a possible type but the problem is that the used calls to
sample
, e.g.sbi/sbi/neural_nets/mnle.py
Line 265 in c347d5c
are not compatible to your
DensityEstimator
.utils/user_input_checks.py
anduser_input_checks_utils.py
:sbi/sbi/utils/user_input_checks_utils.py
Line 151 in c347d5c
You assume that univariate and multivariate random variables have the same interface. I've already added a failing test in b455a7f
LazyTransform
andAffineTransform | Unconditional
are compatible.Feel free to fix things directly in this PR or create issues for those problems. We won't be able to merge this PR before fixing them.
Fixes #847
Checklist
guidelines