search

desihub / specter

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

Does specter explicitly require fitsio, or just a FITS reader? #14

Closed weaverba137 closed 8 years ago

weaverba137 commented 8 years ago

According to our (admittedly informal) software policies, we are only supposed to use fitsio over astropy.io.fits if it is absolutely necessary for speed. Is this the case in specter?

sbailey commented 8 years ago

Ah, dangling threads! the branch astropy_fits converted from fitsio to astropy.io.fits 2 months ago, but I never submitted a pull request for it. I don't recall why. I'll merge in your latest changes (after reviewing them) and then revive this branch and merge it in.

sbailey commented 8 years ago

Reopening issue and changing label to enhancement to flag the needed action of merging astropy_fits into master.

weaverba137 commented 8 years ago

I'd like this merge to happen before I update the specter code to the latest version of desitemplate. One problem is that currently specter depends on both fitsio and astropy.io.fits. You can see this in py/specter/test/test_binscripts.py.

As far as I can tell though, specter only depends on these external packages:

sbailey commented 8 years ago

PR #19 replaces fitsio with astropy.io.fits.

I hadn't noticed that desiutil had slipped in as a dependency (though I admit I reviewed the PR that brought it in). specter should remain experiment agnostic, and thus not depend upon desiutil even if that requires replicating some utility code such as the version string parsing. I'll open a separate ticket about that. I'm not sure what the implications are for desiInstall, but please don't bring in any additional uses of desiutil. OK to otherwise standardize how sphinx docs are configured, etc.