fix: removing circular imports

manuelgloeckler commented 3 months ago

What does this implement/fix? Explain your changes

Currently, a lot of functions can not be directly imported due to circular imports e.g.

from sbi.neural_nets.factory import posterior_nn
from sbi.neural_nets.categorial import build_categoricalmassestimator
from sbi.neural_nets.mnle import MNLE
...

Does this close any currently open issues?

Maybe #1158

Any relevant code examples, logs, error output, etc?

This is likely due to our current import hierarchy, which is quite chaotic and circular. packages

manuelgloeckler commented 3 months ago

Internal neural_nets module

packages

Edit updated:

packages

manuelgloeckler commented 3 months ago

Just for 'uitls` module packages

Edit: This I already fixed and seems fine.

manuelgloeckler commented 3 months ago

Just for inference

packages

Edit: Updated packages

manuelgloeckler commented 3 months ago

Just for analysis

packages

Edit: Fixed packages

manuelgloeckler commented 3 months ago

Just for samplers

packages

Edit: This seems already good

manuelgloeckler commented 3 months ago

Okay what's left over (according to pylint) currently is:

[x] sbi/simulators/init.py:1:0: R0401: Cyclic import (sbi.inference -> sbi.inference.snpe.snpe_c -> sbi.inference.snpe.snpe_base -> sbi.inference.base) (cyclic-import)
[x] sbi/simulators/init.py:1:0: R0401: Cyclic import (sbi.inference -> sbi.inference.snre -> sbi.inference.snre.snre_a -> sbi.inference.snre.snre_base -> sbi.inference.base) (cyclic-import)
[x] sbi/simulators/init.py:1:0: R0401: Cyclic import (sbi.inference -> sbi.inference.snre -> sbi.inference.snre.snre_c -> sbi.inference.snre.snre_base -> sbi.inference.base) (cyclic-import)
[x] sbi/simulators/init.py:1:0: R0401: Cyclic import (sbi.inference -> sbi.inference.snre -> sbi.inference.snre.snre_b -> sbi.inference.snre.snre_base -> sbi.inference.base) (cyclic-import)
[x] sbi/simulators/init.py:1:0: R0401: Cyclic import (sbi.inference -> sbi.inference.snre -> sbi.inference.snre.bnre -> sbi.inference.snre.snre_a -> sbi.inference.snre.snre_base -> sbi.inference.base) (cyclic-import)
[x] sbi/simulators/init.py:1:0: R0401: Cyclic import (sbi.inference -> sbi.inference.snle.mnle -> sbi.inference.snle.snle_base -> sbi.inference.base) (cyclic-import)
[x] sbi/simulators/init.py:1:0: R0401: Cyclic import (sbi.inference -> sbi.inference.snle.snle_a -> sbi.inference.snle.snle_base -> sbi.inference.base) (cyclic-import)
[x] sbi/simulators/init.py:1:0: R0401: Cyclic import (sbi.inference -> sbi.inference.snpe.snpe_b -> sbi.inference.snpe.snpe_base -> sbi.inference.base) (cyclic-import)
[x] sbi/simulators/init.py:1:0: R0401: Cyclic import (sbi.inference -> sbi.inference.snpe.snpe_a -> sbi.inference.snpe.snpe_base -> sbi.inference.base) (cyclic-import)

Edit: Will still be detected, but now not executed on initialization.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 89.87342% with 8 lines in your changes missing coverage. Please review.

Project coverage is 72.92%. Comparing base (8efb8e4) to head (30d878d). Report is 3 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1179 +/- ## =========================================== - Coverage 83.69% 72.92% -10.77% =========================================== Files 93 94 +1 Lines 7397 7451 +54 =========================================== - Hits 6191 5434 -757 - Misses 1206 2017 +811 ``` | [Flag](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev) | `72.92% <89.87%> (-10.77%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev) | Coverage Δ | | |---|---|---| | [sbi/analysis/plot.py](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179?src=pr&el=tree&filepath=sbi%2Fanalysis%2Fplot.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev#diff-c2JpL2FuYWx5c2lzL3Bsb3QucHk=) | `41.79% <100.00%> (-19.54%)` | :arrow_down: | | [sbi/inference/\_\_init\_\_.py](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179?src=pr&el=tree&filepath=sbi%2Finference%2F__init__.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev#diff-c2JpL2luZmVyZW5jZS9fX2luaXRfXy5weQ==) | `100.00% <100.00%> (ø)` | | | [sbi/inference/abc/\_\_init\_\_.py](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179?src=pr&el=tree&filepath=sbi%2Finference%2Fabc%2F__init__.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev#diff-c2JpL2luZmVyZW5jZS9hYmMvX19pbml0X18ucHk=) | `100.00% <100.00%> (ø)` | | | [sbi/inference/abc/mcabc.py](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179?src=pr&el=tree&filepath=sbi%2Finference%2Fabc%2Fmcabc.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev#diff-c2JpL2luZmVyZW5jZS9hYmMvbWNhYmMucHk=) | `15.87% <100.00%> (-68.00%)` | :arrow_down: | | [sbi/inference/abc/smcabc.py](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179?src=pr&el=tree&filepath=sbi%2Finference%2Fabc%2Fsmcabc.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev#diff-c2JpL2luZmVyZW5jZS9hYmMvc21jYWJjLnB5) | `12.44% <100.00%> (-69.73%)` | :arrow_down: | | [sbi/inference/base.py](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179?src=pr&el=tree&filepath=sbi%2Finference%2Fbase.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev#diff-c2JpL2luZmVyZW5jZS9iYXNlLnB5) | `92.72% <100.00%> (ø)` | | | [sbi/inference/posteriors/base\_posterior.py](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179?src=pr&el=tree&filepath=sbi%2Finference%2Fposteriors%2Fbase_posterior.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev#diff-c2JpL2luZmVyZW5jZS9wb3N0ZXJpb3JzL2Jhc2VfcG9zdGVyaW9yLnB5) | `86.04% <100.00%> (+1.70%)` | :arrow_up: | | [sbi/inference/posteriors/direct\_posterior.py](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179?src=pr&el=tree&filepath=sbi%2Finference%2Fposteriors%2Fdirect_posterior.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev#diff-c2JpL2luZmVyZW5jZS9wb3N0ZXJpb3JzL2RpcmVjdF9wb3N0ZXJpb3IucHk=) | `98.79% <100.00%> (+0.43%)` | :arrow_up: | | [sbi/inference/posteriors/ensemble\_posterior.py](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179?src=pr&el=tree&filepath=sbi%2Finference%2Fposteriors%2Fensemble_posterior.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev#diff-c2JpL2luZmVyZW5jZS9wb3N0ZXJpb3JzL2Vuc2VtYmxlX3Bvc3Rlcmlvci5weQ==) | `50.00% <100.00%> (-36.87%)` | :arrow_down: | | [sbi/inference/posteriors/importance\_posterior.py](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179?src=pr&el=tree&filepath=sbi%2Finference%2Fposteriors%2Fimportance_posterior.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev#diff-c2JpL2luZmVyZW5jZS9wb3N0ZXJpb3JzL2ltcG9ydGFuY2VfcG9zdGVyaW9yLnB5) | `54.68% <ø> (-0.87%)` | :arrow_down: | | ... and [27 more](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev) | | ... and [11 files with indirect coverage changes](https://app.codecov.io/gh/sbi-dev/sbi/pull/1179/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev)

manuelgloeckler commented 3 months ago

Wow, that worked better than expected. All imports work now. Some remarks:

Try to avoid importing high level modules such as import sbi.utils as utils (this has a lot of dependencies) for internal imports. Try to use the low-level children i.e. from sbi.utils.xxx import yyy.
I added a tests to PyTest which will fail if someone introduces new circular import. (Edit: I find this better than pylint/pyright they did not catch all the errors, but stuff that is practically not possible)

manuelgloeckler commented 3 months ago

@janfb Good point; after importing "hierarchically," it looks way better. (at least on the bottom)

I also removed the typing imports because it seems a bit weird to me to expose types from the typing module to the user in sbi.inference (but I can add them back).

packages

manuelgloeckler commented 3 months ago

I removed a lot of from sbi.utils import ___few_functions___, to decrease the large dependency on initializing the whole utils module. Is now more distributed through the individual files

packages

sbi-dev / sbi