search

desihub / specter

A toolkit for simulating multi-object spectrographs
Other
8 stars 7 forks source link

Do load_throughput and load_psf need scipy in order to function? #28

Closed weaverba137 closed 8 years ago

weaverba137 commented 8 years ago

It seems like it should be possible for desimodel to import the load_throughput() and load_psf() functions it needs from specter without needing scipy installed, but maybe the dependency is buried deep within the other functions or classes that these functions themselves call.

sbailey commented 8 years ago

The scipy import in throughput.py was unnecessary and has been removed in branch less-scipy.

I removed some other extraneous scipy imports too. Although scipy isn't needed for load_psf() itself, it is needed for doing anything useful with the PSF object that is generated.

Guessing: is this question motivated by wanting to test desimodel.io.load_psf() without having to install scipy for Travis tests?

weaverba137 commented 8 years ago

That is exactly the motivation.

sbailey commented 8 years ago

Given that scipy is one of our full fledged core dependencies, I'm hesitant to refactor its usage in specter just to make desimodel slightly faster to test with Travis. End users of desimodel will almost certainly have scipy installed, so I think it is only an issue of having travis also install scipy when testing desimodel. If it has consequences beyond that, let me know.

Note: even if we did this "hide scipy imports" refactor now, it would be a fragile solution — developers of specter (a non-DESI-specific package) could bring in valid new scipy imports without realizing that they were breaking desimodel tests. I admit that scipy seems like a overly heavyweight dependency for desimodel, but pragmatically let's just add scipy to the desimodel pre-test installs.

weaverba137 commented 8 years ago

That's fine. Scipy is already being installed in desimodel tests. I just wanted to make sure that it was a genuine dependency.

dkirkby commented 8 years ago

What is the cost of using scipyin travis tests? I am doing this in several packages that genuinely need scipy, but wasn't aware that this had a significant impact. Aren't there any standard packages that travis has pre-installed?

Another travis performance-related issue is how the "Build pushes" and "Build pull requests" options are set. They are both on by default, which ends up duplicating identical tests each time I push a PR branch to github as long as the master branch has not changed since I created the branch.

weaverba137 commented 8 years ago

There isn't that much impact. It has more to do with tracking the full stack of dependencies. It's kind of annoying to successfully install specter, then have a desimodel test fail because scipy is missing.

sbailey commented 8 years ago

I find it handy to run travis tests on branches even before they become PRs. e.g. sometimes I'm debugging the travis tests themselves, other times it is trying numpy/astropy/scipy version combinations that I'm not using locally. It's nice to know that all tests are passing prior to submitting the PR. So I suggest leaving both "build pushes" and "build pull requests" on, even if they are sometimes redundant, unless there is some other consequence of which I'm unaware (or if I misunderstand the issue @dkirkby is raising).

dkirkby commented 8 years ago

I agree that the tests are useful and we want to encourage people to create PRs early in the process of working on a new issue. I was just pointing out that in the most common scenario, the default settings lead to running an identical set of tests twice (so do not provide any additional info).

If Travis performance becomes a bottleneck in future, we could revisit our policy on these config options.