Skip to content
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

Merged
merged 3 commits into from
Feb 7, 2024
Merged

Remove jdaviz #141

merged 3 commits into from
Feb 7, 2024

Conversation

JarronL
Copy link
Collaborator

@JarronL JarronL commented Jan 30, 2024

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.

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).
@mperrin
Copy link
Collaborator

mperrin commented Feb 2, 2024

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
Copy link
Collaborator Author

JarronL commented Feb 5, 2024

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.

Copy link
Collaborator

@kammerje kammerje left a 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.

@kammerje kammerje merged commit 75b285b into develop Feb 7, 2024
4 checks passed
@mperrin mperrin deleted the remove_jdaviz branch July 31, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants