Multiple improvements for log scale support#646
Multiple improvements for log scale support#646diorcety wants to merge 22 commits intopyqtgraph:masterfrom
Conversation
7f0d367 to
52ec289
Compare
pyqtgraph/functions.py
Outdated
| rng = 1 if rng == 0 else rng | ||
| d2 = (data - float(minVal)) * scale / (rng) | ||
| else: | ||
| rng = np.log10(maxVal)-np.log10(minVal) |
There was a problem hiding this comment.
if dealing with singular values, math.log10 is far faster than np.log10
|
This PR is a doozy due to the size of the diff, and the number of conflicts. I've resolved them locally, but the diff is way too large to just squash on merge IMO. I'm going to leave this open, but highly recommend not merging. Meanwhile I'll try and submit a series of PRs that replicate the functionality/fixes that this PR brings. Sorry @diorcety for not getting to this earlier. The size of this PR made it so we kept pushing back. Thanks for the submission, I'll try and cherry pick your commits in the construction of a series of smaller PRs once I get the test suite to pass. |
|
I think this is as far as I can carry this PR with respect to the test suite, pyside pipelines are passing, pyqt pipelines are failing some of the Once I verify behavior/functionality is there, I'm going to start taking bits and pieces of this PR into smaller, easier to manage PRs. |
|
and by shear-dumb-luck I came across the line that is breaking test_ref_cycles I've always heard that connecting lambdas to signals wasn't a great idea, guess now I can see that first-hand... |
I am not very familiar with the way lambda capturing works, so I would normally code lambdas this way: self.vb.sigLogChanged.connect(lambda x, self=self: self.setLogMode(x.xLog(), x.yLog())) |
I hadn't seen that pattern before, tried it ultimately to the same result... Doing some quick searching yields this, suggesting it's not garbage collected https://stackoverflow.com/a/48501804 |
|
Now all tests pass, no warnings are emitted... can finally start evaluating how well this all works 😆 The diff isn't actually that large. If anyone here is interested in log-mode, I would encourage you all to take a look at this PR. @dgoeries I think you had an interest in log-mode stuff. |
| rng = maxVal-minVal | ||
| rng = 1 if rng == 0 else rng | ||
| data = rescaleData(data, scale/rng, minVal, dtype=dtype) | ||
| data = rescaleData(data, scale/rng, minVal, dtype=dtype, log=log) |
There was a problem hiding this comment.
I think these two changes will be insufficient to have makeARGB properly support log-scale image processing. Are the lut and levels args assumed to have already been adjusted to account for the log scale? I think we should omit changes to this method entirely, rather than implement them halfway and imply otherwise.
There was a problem hiding this comment.
Though this may be more sufficient than I understand. Providing an example use or test would help me. For now, though, if a user wants to process the image with log10, my first impulse would be to recommend they do so before sending it to makeARGB, and they could, at that time, adjust lut and levels to match their expectations.
There was a problem hiding this comment.
I am not that familiar with how the processing in ImageItem works, but my understanding is that moving the log10 inside rescaleData would also require a recalculation of the logarithm on each re-render, even if the image hasn't changed. I suspect that this would make interactive adjustments to the LUT slower than necessary.
| if log: | ||
| with np.errstate(invalid='ignore', divide='ignore'): | ||
| np.log10(d2, out=d2) | ||
| d2 -= math.log10(offset) |
There was a problem hiding this comment.
What about when offset is 0? This would raise a ValueError.
| return matrix | ||
|
|
||
| def rescaleData(data, scale, offset, dtype=None, clip=None): | ||
| def rescaleData(data, scale, offset, dtype=None, clip=None, log=False): |
There was a problem hiding this comment.
Oof; log can also refer to logging. I think renaming it to something more specific would be helpful. logScale, log10, logarithmic come to mind.
|
Ok, I'm leaning on scraping this PR then. It's a shame, there is a lot of good stuff here, but between no docs, no tests, no examples, the size of the diff, and not addressing any of the current open issues; I think we should leave this guy perhaps in draft form (or close it out entirely) and make a series of smaller PRs that perhaps use bits of this code while addressing some of the currently open issues. |
|
Sorry for the noise. The changes are too heavy and I have no time to add tests and other work ( and I have no more need of this also) |
|
Hi @diorcety I just realized you must have been getting pinged for updates on your PR, sorry about that, I didn't actually intend to bug you about it. So much time had passed since you submitted it since we got to looking more closely at it, that we didn't expect you to address issues like merge conflicts and such. Unfortunately you submitted this PR at a time when the library was more or less on life support. Since development picked up, we have been looking to get the outstanding PR count down; which meant larger PRs such as this one kept getting postponed in their evaluation. Thanks for the PR, I'll likely use some of the diff here as a starting place as I look to address outstanding issues involving log-support, as that functionality clearly doesn't work as well as it should. |
Support log in HistogramLUTItem
Add minLevel, maxLevel edit box in GradientEditor
Small improvements in the behaviours
Replace #411