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

Brightness & Contrast window , suggestions #26

Closed
romainGuiet opened this issue Nov 21, 2016 · 16 comments
Closed

Brightness & Contrast window , suggestions #26

romainGuiet opened this issue Nov 21, 2016 · 16 comments
Assignees

Comments

@romainGuiet
Copy link

Hi Peter,
about the Brightness & Contrast window

  • would it be possible to set values with numeric fields ? (on top of using sliders)
  • that theses settings are saved within the qpdata file of the image (and thus reused when you re-open it). Up to now the settings are lost if you close and open again the program.
  • could there be an option to use the same values for all the images within a project ?

bandc_qupath

@petebankhead
Copy link
Member

Hi Romain,

You should be able to double-click on the 'Min display' or 'Max display' labels and input values manually there.

As you've likely seen, the values should persist when the image is closed/reopened within the same QuPath session, but not when QuPath is reopened.

I'll give some thought to where this information could best be added to the .qpdata file, and how to set the values through scripting (similar to ImageJ's setMinAndMax(min, max, channels). If both of these can be done, then running such a script would be one way to use the same values across the whole project.

I'll post an update here when I've made progress on this.

@romainGuiet
Copy link
Author

I tried to double-click on the numbers (bottom of the window) but not on "Min display" nor "May display".
If there are some others "hidden functions" it could be great to have a typo (bold or underlined) or a "onmouseover" change.

@petebankhead
Copy link
Member

Oh yes, there are shortcuts & tricks everywhere... documentation hasn't caught up yet, so some things still rely upon someone asking :)

This one was only added recently because I wanted to go beyond the range the sliders would allow. It is (only) described at the very end of this new addition to the Wiki: https://github.com/qupath/qupath/wiki/Changing-colors

I've added a tooltip for the labels in the meantime to make it slightly easier to find. Longer term, I hope to have a more prominent way of setting the min/max values.

@belliveau13
Copy link

As you've likely seen, the values should persist when the image is closed/reopened within the same QuPath session, but not when QuPath is reopened.

Hi! @petebankhead I was just wondering if there's been any updates to this thread? Is there a way to save the max/min values to be used once QuPath is reopened - for example to apply these settings to an entire project?

Thank you,

belliveau13

@petebankhead
Copy link
Member

Yes and no... not in any publicly available way, and it remains on my todo list before the next release. This is taking longer than I'd hope as I find more and more to do, but some talks and workshops in March give me a pretty hard deadline....

However, amidst all the changes I'm making for the next release I've introduced an option to 'retain the display settings' when opening a new image. This means that if you have an image open with certain brightness/contrast settings (and color channels turned on/off), then if you open another image in the project it will (optionally) keep the settings constant:
petebankhead@5750e42

My hope is that it reduces the need to apply settings across all images in a project through a script, because the min/max settings (optionally) don't automatically change per image. How does that sounds to you?

Alongside that, I've added the ability to turn on/off multiple channels at once (by selecting them and right-clicking), and given a bit more control on what the 'Auto' button does when adjusting brightness/contrast per channel. I think these changes make the Brightness/Contrast dialog considerably easier to use; at least, I find it less annoying now than it previously was.

(I still do need to revisit saving settings though, because it needs to be possible to set channel names if these are wrong or missing. And if it's possible to save channel names, it may as well be possible to save display settings per channel as well...)

@belliveau13
Copy link

@petebankhead this sounds super reasonable!! Thank you for your quick response and for your amazing work!

@romainGuiet
Copy link
Author

romainGuiet commented Jan 14, 2019

Hi @petebankhead , Hi @belliveau13

The growing need coming from our users and lacan's curiosity for the QuPath "extension" made him write a tool which allow the user to :

  • Save/Load the current display settings
  • Apply the settings to the similar images in the project

From @lacan : “It requires QuPath 0.1.4, which is a minor update released by our group, that has a few functions made public. We’ve also created a small extension (which is currently only compatible with v0.1.4) that can handle saving and reapplying brightness and contrast settings (NEED DOC).
Howeever, we would like to point out that you can use this version at your own risk. We will, of course merge all we can with @petebankhead’s new and coming release and modify what we need, but some functionality may be broken in between.

In case you are interested, you can find some links on our documentation page

Best,

Romain

image

@Svidro
Copy link

Svidro commented Jan 14, 2019

Thanks! It would be great to take a look at an extension created by a user. I never managed to do it myself. Never created a script with a UI until I had a few examples to look through either!

Thanks for the work @lacan

@petebankhead
Copy link
Member

Closing this due to lack of activity, and because at least some of the issues have been addressed.

I still don't think the brightness/contrast handling in QuPath is ideal, but addressing this requires a more thorough rethink and I don't think it can be achieved by incremental adjustments. So that remains a task for a future version...

@NicoKiaru
Copy link

Hi @petebankhead,

RGB may be the main target of QuPath and they have no such issue. But honestly the lack of ability to conveniently set B&C for all fluorescent images of the same kind is pretty annoying. We regularly have users with k.10's of multichannel images.

The workaround with the retain option is okish but clearly not as convenient as a 'set B&C for all images of the same kind'.

So that remains a task for a future version...

So why not letting the issue open and add a tag for a future release ?

@petebankhead
Copy link
Member

Hi @NicoKiaru, I closed this because it became fragmented and hard for follow, and there have been no comments for over a year. Without any clear feedback I couldn't know that it is pretty annoying for your users.

In case you missed it, I already added scripting methods to help: petebankhead#37 (comment)

@lacan
Copy link
Contributor

lacan commented May 22, 2020

@petebankhead the last comment before it became closed came from @romainGuiet from which we heard no comments from you. Admittedly we did not pursue this further as we had a workaround in our version.
I would be curious to have your opinion on how we did it and whether you think that the approach we took could break anything. The commands we built were pretty simple and were not breaking anything. they were of course not polished to look pretty but the desired effect and user satisfaction were worth the effort we had put into it.
You can find the commands here:
https://github.com/BIOP/qupath-biop-extensions/blob/master/src/main/java/ch/epfl/biop/qupath/commands/ApplyDisplaySettingsCommand.java

There are two other commands in that directory that do similar things, but that were not so used in the end. The one people really liked was the one linked above.

These are not doing or hacking anything special in QuPath to make them work, and having them in the QuPath main code would remove the package private issues I had to deal with.

@petebankhead
Copy link
Member

As I mentioned (each time you brought this up :) ) the approach you have taken creates a dependency on ImageDisplay that I really do not want to be stuck with. It creates an awkward confusion between the GUI and core code that will be a maintenance headache, and would greatly complicate trying to implement a better design later.

I added the alternative scripting methods that I linked to before precisely because you asked for it. Running that for a project is the solution I propose. It uses ImageDisplay internally (because it has to), but doesn't expose this publicly.

Romain's comment wasn't a question, it seemed you had a solution you were satisfied with, and I received no reply to the changes I made for you except for 👍 so it remains very unclear to me what you want...

@lacan
Copy link
Contributor

lacan commented May 22, 2020

Thank yo for the reply and your re-repeat :) of what you had already told me. I admit that I am really bad at understanding this part. This is what I think I understand:

The approach we used creates an issue with the way ImageDisplay is currently implemented because saving display settings to the .qpdata file is not good practice?
But doing this via scripting is OK because it's less important if scripts break upon QuPath updates?

The risk is that if these are made public, other people could call upon these methods, and that would break something in the GUI whenever changes will be made to the code?

Currently there is no other way than to use ImageDisplay to set these properties for the channels (and save and recall them) which is bad because this will be revised in the future?

Let me know what I got wrong there if you have time. I think that in the meantime I will test a script to do what we used to be able to do, and dive into the wonderful world of Reflection for my sake.

@petebankhead
Copy link
Member

Well, you can avoid both reflection and scripting if you go to QPEx.... but no absolute promises that won't be broken in the future either :)

Ok, I will explain some more and hope it will be sufficient.

QuPath is divided into modules. This modular design is a work-in-progress, but it is essential to keep the design coherent/improve it where possible.

ImageDisplay requires JavaFX. That means that using it in any module will bring in a (quite huge) JavaFX dependency to that module. That means the core modules (which are currently completely ignorant of JavaFX) cannot use ImageDisplay... or they suddenly become dependent upon a whole host of other stuff. This is problematic if wanting to use some QuPath jars in other contexts in the future.

Of course, ImageData exists in a core module. Currently, these means that if serializing the ImageDisplay inside the ImageData, the ImageData ends up storing a JSON version of something that it cannot possibly de-json-ify. This is tolerable, but not ideal.

More critically, it also means that nothing in core modules can really work with the current display or channel settings. Perhaps they would like to, e.g. to export RGB image regions. Ideally this would not be restricted to modules that have JavaFX access. It also complicates things like the ImageJ macro runner... currently, this can either be free from JavaFX or capable of incorporating color transforms/channel info - but not both. There are good reasons to want both #68

Also, it means that changing the brightness and contrast ultimately requires deserializing/serializing the whole data file - which might be large. There are likely far better/more efficient/faster ways to store these settings in a project, not the data file. This would not only be arguably a a better design, but it would also make updating this information for 10,000 images almost instantaneous.

I have made some progress in parts of this, because I needed a way to have JSON-serializable color transforms separate from the GUI in order to support stain separation in the pixel classifier and thresholder.... which was needed to make the sluggish and limited 'Positive pixel counter' unnecessary... which was needed for my sanity so I'd have to stop answering questions about such a poorly-implemented command (that I had originally implemented).

Doing this involved writing a completely separate way of representing the transforms than the one used by ImageDisplay. Eventually I think this kind of color transform approach should completely replace the (currently GUI-only) color transforms for better consistency and more maintainable code.

More generally, I need to be thinking broadly about existing users as well as how the software will need to look a year from now, or two years from now, to meet a host of new applications. And I need to think about how much of those years will go into maintaining existing things where it is already clear they are not using the right approach. Often, there are lots of considerations that I haven't articulated anywhere (there just isn't time), but which are impacted by the choices.

For example: finding a better approach to handle brightness/contrast perhaps could/should also support serializing the image histograms (since ImageDisplay uses them). Storing these histograms would make opening images a great deal faster as well.

But then, having histograms separated from the GUI (and JavaFX) would also make intensity distribution information instantly available in general. This might open up new and faster processing and analysis options - including the use of automated thresholds based on such histograms.

If something is not public, it can be freely changed without breaking other extensions (and also well-behaved scripts). If it is public, other extensions and scripts that use it will definitely break. Each breaking change costs a) user annoyance, and b) developer time. Making fewer things public reduces that.

Time is incredibly precious... there are now (finally) two of us working on it, but there are quite some demands on us. And in academia, a lot of what we are judged on isn't software anyway. So I think it is important we follow our beliefs about what will protect our time and be better in the long run - trying to be helpful, but not caving to pressure :)

So why the scripting approach?

In general, when something is used internally by QuPath, we have a much better idea of what we might be breaking... and when a path needs to be found through the pain (e.g. the ability to import images from v0.1.2 projects to v0.2.0). When it's in the public API, we have no idea how it is used or the implications of our changes.

Because the scripting approach I proposed only uses ImageDisplay internally, so long as any improved approach is capable of supporting a method that does the same thing then we're free to change the method in QPEx without worrying about breaking things for anyone else. We can even move it up into QP so that it works without knowing anything about the viewer at all.

I think that as a compromise this is more than fair. It means you get the outcome you want, and we did not have to compromise to do something that I strongly believe will end up wasting a lot of time in the future (be that mine or someone else's).

QuPath remains a 0.x.x release and so the API shouldn't be interpreted as stable. I don't encourage writing extensions for that reason. But I do recognise that extensions are important, and so if someone wants to do it (aware of the risks) then it is supported.

I hope that more clearly explains my logic.

Since it feels like we've discussed this subject many times, I thought I should be thorough in this answer. Now I've no time to shorten it.... I hope it is useful.

v0.2.0 has been a rather... intense experience. Pretty much the entire software has been rewritten, while still trying to keep it basically functional and respond to the ever-increasing questions and requests from users. Sometimes it gets exhausting.

QuPath is by no means finished, but I do think it is substantially better and more coherent than it previously was. The goal of v0.2.0 was to get decent foundations as quickly as possible - but the task turned out to be huge. The importance of many of the new features will only become clear in future releases. v0.3.0 won't have so many milestones, and I hope will mark the start of a more sustainable development approach...

@lacan
Copy link
Contributor

lacan commented May 22, 2020

Pete,

Thank you deeply for the complete explanation and information regarding this.

I will now consider this as closed myself, but will most likely continue (at my own risk, as you say) working on this feature as I sincerely do believe it to be useful, and will find a way without changing anything in QuPath. I am aware it will break at one point or another but for the benefit of our users it will be worth to maintain.

Thank you for all the time and effort explaining this to me, and for the scripting alternatives you have provided which I am sure we will be making use of. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants