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

Edits to ColorTables of ImagePlus created from a Dataset never make it back to that Dataset #311

Open
gselzer opened this issue Nov 18, 2024 · 4 comments

Comments

@gselzer
Copy link
Contributor

gselzer commented Nov 18, 2024

@ctrueden provided me with a script that can be run in Fiji-future with a SNAPSHOT scijava/scripting-python jar, which does the following:

  • Initializes a multi-channel Dataset.
  • Converts it to an ImagePlus using PyImageJ (which uses the DatasetToImagePlusConverter).
  • Edits the LUT of that ImagePlus, such that one channel is Magenta.
  • Displays the ImagePlus.
    From there, I want to interact with the active ImageDisplay, so I call LegacyImageDisplayService.getActiveImageDisplay. This taps into the LegacyImageMap, finds the associated ImageDisplay within its internal map, and returns it after synchronizing the associated attachments (i.e. ROIs and Tables, but not LUT)s. Therefore, the returned dataset does not have the edited Magenta that I see in the active image display.

LegacyImageMap makes a Harmonizer object on events - is there a reason why it cannot have one of these all the time, such that syncrhonizeAttachmentsToDataset could utilize it?

@ctrueden
Copy link
Member

The reason is that trying to keep ImagePlus and ImageDisplay objects in sync completely is a wild goose chase. Some data structures like the color tables are not wrapped but rather copied, and knowing under which circumstances exactly to trigger automatic sync is fraught: if you do it too often there is a potentially severe performance hit, and if you do it not often enough then things get out of sync. So in recent years we have not tried to improve this. My preference would be to actually back off the syncing approach altogether in favor of code wanting to utilize both kinds of data structures doing it themselves.

@gselzer
Copy link
Contributor Author

gselzer commented Nov 18, 2024

Some data structures like the color tables are not wrapped but rather copied,

Looking a bit at the API, I'm not sure I understand why they couldn't be wrapped - is there a reason that you know?

My preference would be to actually back off the syncing approach altogether in favor of code wanting to utilize both kinds of data structures doing it themselves.

Well, then at the very least we need to expose the logic that is used to perform the conversion, namely Converter plugins that go between ij.process.LUTs and net.imglib2.display.ColorTables.

On a broader scale though, I'm not sure what you mean by "code wanting to utilize both data structures". This repository, pyimagej/napari-imagej, and the example script listed above are all versions of such. If you really think the synchronization code does not belong here, maybe pyimagej is the correct place for this, within ij.py.sync_image? It's really unfortunate, though, that some of the synchronization code is there, and more is here...or do you think the script author should be doing the synchronization?

@ctrueden
Copy link
Member

@gselzer I definitely think all conversion code between ImageJ and ImageJ2 data structures belongs here in imagej-legacy, not in Python code. And code between ImageJ and ImgLib2 data structures belongs in imglib2-ij, which imagej-legacy builds upon.

What I'm saying we should be moving away from is the LegacyImageMap: (ImagePlus, ImageDisplay) pairs that are mutually associated and kept in sync when changes happen on one side or the other. History has shown that it's extremely difficult and fraught with edge cases. It also requires bytecode patching of ImageJ, which has proven to be a huge problem; it is the most common thing developers complain about, especially now that Java 17+ requires --add-opens flags at launch time for it to keep working at all.

I want to interact with the active ImageDisplay, so I call LegacyImageDisplayService.getActiveImageDisplay. This taps into the LegacyImageMap, finds the associated ImageDisplay within its internal map, and returns it after synchronizing the associated attachments (i.e. ROIs and Tables, but not LUT)s.

I'm fine with improving the sync logic to take LUTs into account as well. Just be aware that the logic is fundamentally flawed as written. The LegacyImageDisplayService is a nasty hack that covers a few common cases, triggering sync from ImageJ to ImageJ2 upon certain API calls, but those are far from the only ways one might gain or have access to an ImageDisplay. That said, harmonizing the LUTs along with the ROIs and Tables is certainly something we should be doing, within the current infrastructure.

@gselzer
Copy link
Contributor Author

gselzer commented Nov 19, 2024

I definitely think all conversion code between ImageJ and ImageJ2 data structures belongs here in imagej-legacy, not in Python code. And code between ImageJ and ImgLib2 data structures belongs in imglib2-ij, which imagej-legacy builds upon.

Sounds great!

What I'm saying we should be moving away from is the LegacyImageMap: (ImagePlus, ImageDisplay) pairs that are mutually associated and kept in sync when changes happen on one side or the other.

This is also true, however it's a broader issue that, as you suggest, will be a large endeavor - we may want to think about it eventually, but it isn't a problem to be solved here.

That said, harmonizing the LUTs along with the ROIs and Tables is certainly something we should be doing, within the current infrastructure.

👍

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

No branches or pull requests

2 participants