-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove jdaviz #141
Remove jdaviz #141
Conversation
Instead of requiring users to install all of jdaviz simply for a compass plot, I copied over the 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 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 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. |
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, ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve this PR after iterating with @JarronL on individual modifications. This should make the installation much faster and much lighter.
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.