i.atcorr: fix plot_filter in create_iwave #3911
Conversation
pesekon2
commented
Jun 20, 2024
pesekon2
commented
- missing import of pyplot
- figures not shown correctly
- fix wrong axes
* missing import of pyplot * figures not shown correctly * fix wrong axes
pesekon2
added
bug
There was a problem hiding this comment.
Just like this, this PR changes the behaviour of the function. It's not exactly the same. To know if it's safe and reasonable, I'd need to know a bit more on the context and how it is used.
Is it a function part of "public api", ie used in user code in the wild, or just inside the file?
Before these changes, if I recall correctly, calling plot like this would add/change the currently active plot. In this PR, you are explicitly creating a new one, with new axes.
Before these changes, technically it would be possible to further edit the plot, and would have to call show() when needed (when it was that that was wanted, I usually returned the figure and axes for that in old code I did). Now, the function shows the plot directly here. Since only show() is used, it is a blocking function (I think it depends on the plotting backend though). For example, a non blocking call would be show(block=False) or show(blocking=False) (writing from memory, it's been years). A blocking call prevents other event processing in other parts of the code (but the new window will be able to do the interactions if the backend supports it).
So, just like this, I can't be sure if it's a safe change or not. It's maybe not a dead simple change.
While the authors are free (and welcome) to make it more as an API, the function inside code for individual tools are not part of the API (only the CLI of the tool is), so any change which keeps the tool's functionality (as defined in the CLI) is fine. Similarly, call to show is fine too because the function serves the tool, but is not meant to be used generally at this point (this is acceptable as long as it makes the coding simpler). |
|
Thanks for the reviews. Attaching two screenshots (AVNIR and one of the Planetscopes) to keep them here as an illustration of the function's behaviour.
|
* missing import of pyplot * fix figures not shown correctly * fix wrong axes
