search

kammerje / spaceKLIP

Pipeline for reducing JWST high-contrast imaging data. Published in Kammerer et al. 2022 and Carter et al. 2022.
https://ui.adsabs.harvard.edu/abs/2022SPIE12180E..3NK/abstract
MIT License
16 stars 9 forks source link

Remove jdaviz #141

Closed JarronL closed 6 months ago

JarronL commented 6 months ago

Instead of requiring users to install all of jdaviz (which has a very large number of dependencies) simply for a compass plotting tool, I copied over the standalone wcs_utils.py file which houses the WCA coordinate transformation used by jdaviz to plot the compass. This was originally adapted from Ginga (ginga.util.wcs, ginga.trcalc, and ginga.Bindings.ImageViewBindings).

I'm not sure what the software licensing rules are here, but I would rather figure out a way of ditching jdaviz import since it's not used for anything other than plotting some compass symbols.

mperrin commented 6 months ago

I'm ambivalent on this. In general I think it's better to not proliferate more code or more copies of code, when possible. And a one-line import jdaviz leaves the spaceKLIP codebase simpler and cleaner than copying an entire new file and adding a bunch of code.

What makes it undesirable to import jdaviz from your perspective? You say it's a 'huge package', but I just checked and it's like 6 MB, which is negligible compared to any JWST data files. Or are you referring in turn to its dependencies?

Meanwhile, I have recently been informed there's a PR into astropy to add a compass plotting function there, but it's sort of stalled out since last fall. So, no idea what the timescale for that would be but probably not soon.

JarronL commented 6 months ago

Right, I suppose I could have been more clear. I was referring to the many dependencies foisted upon the user, which ended up breaking my conda configuration on a couple occasions with dependency conflicts. It certainly wasn't a light installation, even if jdaviz's own code is relatively compact. I don't really use jdaviz that often and tend to keep a separate installation isolated in its own environment for when I do want to use it.

My philosophy here is that jdaviz is not a critical dependency for spaceKLIP; we don't use any of it's core features. Indeed, the only jdaviz code that spaceklip calls (which itself appears to be borrowed / ported code from another package) is a minor compass plotting function primarily for display purposes. If it were benign, I wouldn't bother, but having run into issues in the past and having heard a couple of colleagues complain, it seemed like we could eliminate it from the requirements.

I agree that I don't like proliferating copies of code, but figured this could be a quick temporary solution until a more permanent one is implemented. I don't think writing our own version would be that difficult, (webbpsf_ext.coords.plotAxes() is something that could be used adapted, for instance), it's just a matter of finding the time.