Starfish-develop / Starfish

Tools for Flexible Spectroscopic Inference
https://starfish.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
69 stars 22 forks source link

Platform dependent extra_compile_args. #123

Closed jason-neal closed 5 years ago

jason-neal commented 5 years ago

This removes the compile args for a install on windows.

I have tested pip installing this branch successfully with appveyor here

I see in #87 @mileslucas works on a windows machine, how did you get around the compile arg issues. I have it on my windows tablet and on appveyor e.g. here

Command "C:\Python36\python.exe -u -c "import setuptools, tokenize;__file__='C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\pip-install-k56ib4ut\\Starfish\\setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" install --record C:\Users\appveyor\AppData\Local\Temp\1\pip-record-7fltmuk4\install-record.txt --single-version-externally-managed --compile" failed with error code 1 in C:\Users\appveyor\AppData\Local\Temp\1\pip-install-k56ib4ut\Starfish\
Command exited with code 1

I wonder what is different for you if. you do not get these errors.

mileslucas commented 5 years ago

That’s quirky, huh. My machine has up to date installs of visual studio c compiler so that could be it. I also develop using the git bash which uses MinGW to emulate bash, although I’ve been able to install using power shell and the built in terminal for PyCharm.

I also want to note that as I keep rebuilding parts of the code I want to override the cython covariance module with PyTorch Gaussian process kernels. See the discussion of the emulator refactor for that. This would remove all of the cython building and greatly reduce complexity as a trade off for users having to manually install PyTorch.

jason-neal commented 5 years ago

Well if it is going to be eventually removed, this PR doesnºt need to be included and I can just point my use case to this branch for the time being.

iancze commented 5 years ago

I'm fine with seeing the covariance.pyx module go (and with it, the cython dependency) if @mileslucas comes up with a solid PyTorch implementation. The benefit of the covariance.pyx is that I currently understand what's going on, but I'll do my best to learn the relevant parts of PyTorch as Miles implements them.

It seems like PyTorch has a future ahead of it, so I guess it's reasonable to include it as a core dependency. Hopefully we can make this as easy to install as possible to avoid hassling users (any more than they are already hassled by the cython dependency...).

mileslucas commented 5 years ago

So I've been really tied up with the emulator but I'm starting to consider looking at the Order likelihood model and think about how to recreate it in PyTorch or some other library. I think the final decision to see if PyTorch should be used will definitely have to be based on the speed and performance of the MAP estimations and Samples from the MCMC. I think in either case there will be some third-party solution to GP kernels and conditional marginal likelihoods for multivariate normal distributions.