odlgroup / odl

Operator Discretization Library https://odlgroup.github.io/odl/
Mozilla Public License 2.0
370 stars 105 forks source link

Reload modules in Spyder causes bugs #584

Open adler-j opened 8 years ago

adler-j commented 8 years ago

With reload modules on in Spyder, we can get bugs where isinstance(RealNumbers(), RealNumbers) does not seem to hold, at least if the parameters are given in a more complicated manner.

kohr-h commented 7 years ago

Any news on this issue? Can we do anything about it?

adler-j commented 7 years ago

It is likely due to circular dependencies, but no I haven't researched it more. To the best of my knowledge it is still an issue.

Some things that I get if I enable UMR:

Exception AttributeError: "'NoneType' object has no attribute 'data2d'" in <bound method AstraCudaBackProjectorImpl.__del__ of <odl.tomo.backends.astra_cuda.AstraCudaBackProjectorImpl object at 0x000000000C3859E8>> ignored
Exception AttributeError: "'NoneType' object has no attribute 'data2d'" in <bound method AstraCudaProjectorImpl.__del__ of <odl.tomo.backends.astra_cuda.AstraCudaProjectorImpl object at 0x000000000C385128>> ignored

I also get this "reloaded modules":

Reloaded modules: odl.solvers.functional.functional, odl.discr.discr_mappings, odl.set.space,
 odl.trafos.backends.pyfftw_bindings, odl.util.numerics, odl.phantom.geometric, odl.trafos.fourier,
 odl.operator.operator, odl.diagnostics.examples, odl.tomo.analytic.filtered_back_projection,
 odl.tomo.backends.astra_cpu, odl.deform.linearized, odl.trafos.backends.pywt_bindings, odl.discr.partition,
 odl.trafos.util.ft_utils, odl.solvers.functional, odl.solvers.nonsmooth.proximal_gradient_solvers, odl.tomo.data,
 odl.tomo, odl.tomo.operators.ray_trafo, odl.tomo.backends.stir_bindings, odl.operator.pspace_ops, 
 odl.solvers.functional.example_funcs, odl.solvers.nonsmooth.chambolle_pock, odl.solvers, odl.set.sets, 
 odl.solvers.smooth, odl.phantom.emission, odl.space.fspace, odl.util.testutils, odl.tomo.backends.astra_cuda, 
 odl.tomo.geometry.geometry, odl.space.base_ntuples, odl.operator, odl.tomo.geometry, odl.diagnostics.space, 
 odl.operator.tensor_ops, odl.tomo.backends.scikit_radon, odl.trafos.util, odl.util, odl.space.weighting, 
 odl.space.npy_ntuples, odl.discr.discretization, odl.util.utility, odl.tomo.geometry.fanbeam, odl.trafos, 
 odl.phantom.misc_phantoms, odl.phantom.noise, odl.tomo.geometry.conebeam, odl.solvers.nonsmooth.forward_backward, 
 odl.tomo.backends.astra_setup, odl.solvers.iterative, odl.solvers.util.callback, odl.tomo.data.mrc, 
 odl.ufunc_ops.ufunc_ops, odl.solvers.iterative.iterative, odl.solvers.functional.derivatives, odl.discr.grid, 
 odl.tomo.geometry.parallel, odl.util.vectorization, odl.discr.discr_ops, odl.phantom.transmission, 
 odl.util.normalize, odl.phantom.phantom_utils, odl.space.entry_points, odl.space, odl.solvers.smooth.gradient, 
 odl.solvers.functional.default_functionals, odl.deform, odl.diagnostics.operator, odl.phantom, 
 odl.solvers.smooth.newton, odl.tomo.backends, odl.ufunc_ops, odl, odl.trafos.wavelet, odl.tomo.util.utility, 
 odl.trafos.backends, odl.tomo.util, odl.solvers.util.steplen, odl.solvers.nonsmooth, odl.tomo.data.uncompr_bin, 
 odl.tomo.geometry.spect, odl.util.graphics, odl.operator.oputils, odl.util.ufuncs, odl.diagnostics, odl.set, 
 odl.space.space_utils, odl.discr.diff_ops, odl.tomo.operators, odl.solvers.nonsmooth.proximal_operators, 
 odl.tomo.geometry.detector, odl.solvers.iterative.statistical, odl.discr, odl.solvers.util, 
 odl.solvers.nonsmooth.douglas_rachford, odl.tomo.analytic, odl.solvers.smooth.nonlinear_cg, 
 odl.operator.default_ops, odl.discr.lp_discr, odl.set.domain, odl.space.pspace

which some would consider redundant.

kohr-h commented 7 years ago

which some would consider redundant.

That's nicely put :-) So one circular dependency I noticed is operator and space. One would assume that space should come first, but when you use that order in odl/__init__.py the import crashes:


  File "<ipython-input-1-1d1518589b00>", line 1, in <module>
    import odl

  File "/export/scratch1/kohr/git/odl/odl/__init__.py", line 37, in <module>
    from .space import *

  File "/export/scratch1/kohr/git/odl/odl/space/__init__.py", line 25, in <module>
    from . import entry_points

  File "/export/scratch1/kohr/git/odl/odl/space/entry_points.py", line 49, in <module>
    from odl.space.npy_tensors import NumpyTensorSet, NumpyTensorSpace

  File "/export/scratch1/kohr/git/odl/odl/space/npy_tensors.py", line 34, in <module>
    from odl.operator.operator import Operator

  File "/export/scratch1/kohr/git/odl/odl/operator/__init__.py", line 27, in <module>
    from .default_ops import *

  File "/export/scratch1/kohr/git/odl/odl/operator/default_ops.py", line 29, in <module>
    from odl.space import ProductSpace

ImportError: cannot import name 'ProductSpace'

It seems that we've built quite a fragile import chain with these "only-one-level-down imports" like from odl.space import ProductSpace. It's apparently just one addition away from a non-importable package. So the question is if doing that throughout was such a good idea.

adler-j commented 7 years ago

Well I think it is a good design, the main issue is perhaps that we are not following it properly. In my opinion, we should have a rather strict hierarchy, and when broken it should be broken inside functions, i.e. not with top level imports.

In your example, space should not be importing operator stuff.

kohr-h commented 7 years ago

I was struggling a while with a reasonable place for MatrixOperator, which is the culprit in this case -- I ended up putting it back into npy_tensors.py in lack of a better alternative. We should have a better place for operators that use a lot of stuff from the library, as mentioned in #637.

adler-j commented 7 years ago

Well simply making a new file odl/operator/npy_operators.py would perhaps be the best solution in the interim.

kohr-h commented 6 years ago

This should have improved by now. Can you test this @adler-j?