Skip to content

Remove pre-fetch step in measurement exporter#2069

Merged
petebankhead merged 9 commits intoqupath:mainfrom
alanocallaghan:less-mem-measurements
Feb 5, 2026
Merged

Remove pre-fetch step in measurement exporter#2069
petebankhead merged 9 commits intoqupath:mainfrom
alanocallaghan:less-mem-measurements

Conversation

@alanocallaghan
Copy link
Contributor

@alanocallaghan alanocallaghan commented Jan 15, 2026 •
edited
Loading

Remove pre-fetch step in measurement export wherein string representations of all columns were fetched. Now we only pre-query the columns and rows of the table by iterating over the image data. Therefore writing the table will take longer, but without the costly pre-fetch step the overall time may be roughly similar. In theory writing could be parallelized for some formats.

This should also move file creation closer to writing, which should help avoid the situation possible at the moment where we overwrite a file and then crash before writing a single byte to it (if createMeasurementTable fails).

Also, make a small optimisation in writer use going by the javadocs for outputstreamwriter

@alanocallaghan
Copy link
Contributor Author

alanocallaghan commented Jan 15, 2026

Apologies for the unrelated Zarr/H5 stuff, this can be removed or I can try to add these in the same PR as desired.

@alanocallaghan alanocallaghan changed the title Towards a less memory-hungry measurement exporter Remove pre-fetch step in measurement exporter Jan 19, 2026
@alanocallaghan
Towards a less memory-hungry measurement exporter
0caf502 
Attempts to avoid the pre-fetch step when building
by only pre-allocating the measurement tables
and not all of the strings. Therefore writing
the table will take longer, but without the
costly pre-fetch step.
In theory writing could be parallelized for some formats also.

This should also move file creation closer to
writing, which should help avoid the situation
possible at the moment where we overwrite a
file and then crash before writing a single byte to it
without throwing an exception (if createMeasurementTable fails).
@alanocallaghan alanocallaghan marked this pull request as ready for review January 19, 2026 15:59
petebankhead
petebankhead requested changes Jan 26, 2026
View reviewed changes
Copy link
Member

@petebankhead petebankhead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be a big improvement and should help with supporting #2057

Made two comments as I think it could be made quite a lot simpler... or, if not, I'd need more comments to understand why not.

alanocallaghan
alanocallaghan commented Jan 26, 2026
View reviewed changes
qupath-gui-fx/src/main/java/qupath/lib/gui/commands/MeasurementExportCommand.java
exporter.progressMonitor(p -> updateProgress(p, 1.0))
.exportMeasurements(new File(pathOut));
} catch (IOException e) {
logger.error("Export failed", e);
Copy link
Contributor Author

@alanocallaghan alanocallaghan Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously the error message dialog popped up but was instantly nuked by the progress bar closing

Copy link
Member

@petebankhead petebankhead Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... that sounds like maybe we need Dialogs.builder to construct the error dialog and set the owner rather than leaving it up to getDefaultOwner (or improve the logic of getDefaultOwner, but that sounds tricky if it doesn't know we'll promptly be closing the progress bar)

@alanocallaghan
Copy link
Contributor Author

alanocallaghan commented Jan 26, 2026

Okay fair dues, that does indeed make it less convoluted. The progress monitor is now basically unused and has been removed, unless this should be kept? With an iterator it's not clear how to track updates

@petebankhead
Copy link
Member

petebankhead commented Jan 27, 2026

Fair point... I suppose progress could be per ImageData, accepting the potential for jumps if some images contain vastly more objects than others.

Alternatively, you could count the objects for each image initially when building the table and use that to update the progress in one of 2 ways:

  1. Progress is updated once per ImageData, with the number of objects used to weight each progress increment
  2. Progress is updated per row - with the constraint that the progress isn't allowed to hit 100% until the iterator is finished (so conceivably it could 'freeze' if somehow extra objects have appeared... although this isn't expected to ever happen).

petebankhead
petebankhead requested changes Jan 27, 2026
View reviewed changes
Copy link
Member

@petebankhead petebankhead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's improving in terms of becoming simpler, although in its current state seems to be fundamentally broken... both in terms of columns and rows that are export.

@alanocallaghan
Copy link
Contributor Author

alanocallaghan commented Jan 27, 2026

Think that's fixed now, also with progress updated once per image data as before. Although now it is when writing data, not when creating the table of strings.

@petebankhead
Copy link
Member

petebankhead commented Feb 5, 2026

I'll merge this as it certainly improves some important things. I plan to follow up with another PR to handle a few others:

  • Try to stop more elegantly when the user requests 'cancel'
  • Investigate the use of NaN/missing values (might require a change elsewhere)

@petebankhead petebankhead merged commit fa16cf5 into qupath:main Feb 5, 2026
3 checks passed
@alanocallaghan alanocallaghan deleted the less-mem-measurements branch February 5, 2026 17:38
@alanocallaghan
Copy link
Contributor Author

alanocallaghan commented Feb 5, 2026

Neat thanks, I would offer to handle these but I'm sure it's apparent my understanding of the layers here is not comprehensive, to put it politely

@petebankhead petebankhead mentioned this pull request Feb 12, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@petebankhead petebankhead petebankhead approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alanocallaghan @petebankhead