bayesiains / nflows

Normalizing flows in PyTorch
MIT License
849 stars 118 forks source link

UMNN is not included in setup.py #36

Open mj-will opened 3 years ago

mj-will commented 3 years ago

Hi,

When UMNN was added to nflows https://github.com/bayesiains/nflows/pull/29, umnn was only added to environment.yml. So when running pip install . to install the current master version, it's not installed and importing any of the related functions fails.

Whilst the is easily fixed by just installing it manually, I think it might be worth adding it either to the dependencies in setup.py or maybe as an optional requirement in extra_requires.

arturbekasov commented 3 years ago

Hi there,

Yes, this is by intention. I didn’t want to make umnn a strict requirement, as it’s only needed for a small subset of transforms.

Manually installing if/when needed is what I had in mind, but we should’ve documented this better. On the other hand, adding to extra_requires could be a good compromise — I’ll look into this.

Thanks,

Artur

mj-will commented 3 years ago

Hi Artur,

Thanks for getting back about this, it makes sense.

Since opening the issue I've realised that with the current master none of the modules in nflows.transforms (e.g. CouplingTransform) can be imported if UNMM is not installed:

>>> import nflows.transforms
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/michaelwilliams/git_repos/nflows/nflows/transforms/__init__.py", line 1, in <module>
    from nflows.transforms.autoregressive import (
  File "/home/michaelwilliams/git_repos/nflows/nflows/transforms/autoregressive.py", line 21, in <module>
    from nflows.transforms.UMNN import *
  File "/home/michaelwilliams/git_repos/nflows/nflows/transforms/UMNN/__init__.py", line 1, in <module>
    from nflows.transforms.UMNN.MonotonicNormalizer import MonotonicNormalizer, IntegrandNet
  File "/home/michaelwilliams/git_repos/nflows/nflows/transforms/UMNN/MonotonicNormalizer.py", line 2, in <module>
    from UMNN import NeuralIntegral, ParallelNeuralIntegral
ModuleNotFoundError: No module named 'UMNN'

Presumably, this is because they're included in nflows.transforms.__init__.py, so I guess some more changes might be needed to make it an optional dependency.

Thanks,

Michael

arturbekasov commented 3 years ago

Now this isn’t intentional. :-) Thanks for flagging! Yeah, those transforms can’t be in the root init file for transforms module if the dependency is optional. Having a transforms.umnn submodule should resolve this.