search

ejeschke / ginga

The Ginga astronomical FITS file viewer
BSD 3-Clause "New" or "Revised" License
121 stars 77 forks source link

Clarify that opencv acceleration is used by default #957

Closed ejeschke closed 3 years ago

ejeschke commented 3 years ago

Previously, OpenCv support had to be explicitly enabled via a command line switch or option in a configuration file. This was due to a problem importing cv2 that could crash the application on some platforms. The OpenCv project has fixed that bug some time ago and for quite a while now, Ginga has been quietly enabling OpenCv support by default.

This PR removes the --opencv switch from example programs and updates the documentation to indicate that it is used by default if installed. The option is retained on the reference viewer, however it will generate a logger warning that the option has been deprecated.

ejeschke commented 3 years ago

@pllim, I'm going to recommend merging before 3.2 release, since this corrects the documentation for what is already the case.

pllim commented 3 years ago

I think we also have to modify the astrowidgets API to match depending on the Ginga version it pulls. cc @mwcraig

https://github.com/astropy/astrowidgets/blob/ace5abb820c030122add475ecf106114aaa68e1b/astrowidgets/core.py#L63

ejeschke commented 3 years ago

I don't think I have opencv installed either. How will this affect me as a user?

Depending on how intensively you use Ginga, probably wont affect you much. If you do a fair bit of rotation or maybe mosaicing, it might speed things up if you install it.

ejeschke commented 3 years ago

I think we also have to modify the astrowidgets API to match depending on the Ginga version it pulls. cc @mwcraig

https://github.com/astropy/astrowidgets/blob/ace5abb820c030122add475ecf106114aaa68e1b/astrowidgets/core.py#L63

Hmm, except I haven't changed the API at all here.

pllim commented 3 years ago

Re: astrowidgets -- If I understood this PR correctly, we won't need use_opencv=True option in astrowidgets anymore, right?

ejeschke commented 3 years ago

Re: astrowidgets -- If I understood this PR correctly, we won't need use_opencv=True option in astrowidgets anymore, right?

Correct. But I didn't remove it from the API. It will just ignore it for now.

ejeschke commented 3 years ago

It will just ignore it for now.

It was already using OpenCv it you had it installed, no matter what you passed in.

ejeschke commented 3 years ago

@pllim, OK2M?