ejeschke / ginga

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

Add AutoLoad plugin #1030

Closed ejeschke closed 1 year ago

ejeschke commented 1 year ago
ejeschke commented 1 year ago

@pllim, you might find this plugin useful in certain cases. I know you are busy, feel free to review if you have time.

ejeschke commented 1 year ago

I am a little concerned that most users will not be familiar with regex and will accidentally enter something that crashes it. I wonder if it needs to be stricter...

It catches any failure to compile the regex and shows the error message in the "Errors" plugin. I think if you could crash Python by passing a bad regexp to re.compile then Python would have a very serious weakness. I entered a number of bad regexps during my debugging of this and it never crashed. If you pass a regexp that compiles, but doesn't match your filename then simply nothing gets loaded and there is a message in the log saying that the file didn't match the regex.

pllim commented 1 year ago

I was thinking about the case where it matches something that looks like a plugin but the file is actually corrupt.

ejeschke commented 1 year ago

This is for loading data files, not plugins. It doesn't evaluate Python, just tries to open the usual data files that Ginga handles.

ejeschke commented 1 year ago

Like FITS or PNG, etc.

pllim commented 1 year ago

Ah, sorry! I misunderstood. OK, in that case, please ignore me. Sorry for the noise. 😅

p.s. Kinda dangerous when numImages = 0 though. One can run out of memory.

https://github.com/ejeschke/ginga/blob/de4baf921d65afa46b48bc3ce2dd0bdecbcdbf15/ginga/examples/configs/general.cfg#L47-L49

ejeschke commented 1 year ago

Kinda dangerous when numImages = 0 though. One can run out of memory.

True. I thought it was you that asked for 0 to be unlimited though, IIRC--back in the day? These days 10 seems like a lot with huge data files. I've thought about adding a feature that would be based on memory use per channel rather than number of images. But that's another PR.

ejeschke commented 1 year ago

The idea behind this plugin is for when you have a pipeline say, that is processing and spitting finished images into a folder, Ginga can load them automatically into a channel using this plugin.

pllim commented 1 year ago

I thought it was you that asked for 0 to be unlimited though

Yes, I did. We had a use case where user wants to load hundreds of small cutout images but they didn't know how many would come in.

I wonder if autoload should be disabled if unlimited images setting is detected...

ejeschke commented 1 year ago

I wonder if autoload should be disabled if unlimited images setting is detected...

I could add a warning message.

I think since the default is non-zero and the user has to explicitly set it to zero to get unlimited, and it's pretty understandable what "unlimited" means (:smile_cat:) then they could run into trouble just regularly loading images in the normal way. In that case, if they use this plugin that is their decision.

pllim commented 1 year ago

Well... the user could unwittingly inherited a project-wide setting or set it a long time ago and forgot about it.

ejeschke commented 1 year ago

In that case should we issue a warning the first time they load an image into an unlimited channel? What if they set the limit to 1,000,000? It's effectively the same, they could easily run out of memory anytime.

I would suggest maybe adding a separate PR that checks the total size of all images loaded and issues a pop-up warning when that is exceeded, or when memory seems to be running low...

pllim commented 1 year ago

Separate PR is fine. Thanks!

ejeschke commented 1 year ago

Thanks for the review, @pllim!