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

Add reset methods for each option #111

Merged
merged 5 commits into from
Nov 30, 2023

Conversation

melissalinkert
Copy link
Member

Copy link
Member

@DavidStirling DavidStirling left a comment

Choose a reason for hiding this comment

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

Changes look fine, though between this and the getters/setters could I check whether the tileQueue size limit should be being adjusted when maxWorkers is changed? It currently looks like this is only set when the class is initialised.

public PyramidFromDirectoryWriter() {
tileQueue = new LimitedQueue<Runnable>(maxWorkers);
}

@melissalinkert
Copy link
Member Author

Changes look fine, though between this and the getters/setters could I check whether the tileQueue size limit should be being adjusted when maxWorkers is changed? It currently looks like this is only set when the class is initialised.

Good catch; a0dafc7 should reset tileQueue to match maxWorkers when the setter or resetter actually changes the value of maxWorkers.

@DavidStirling
Copy link
Member

I'm a bit lost here now, how are we meant to reset values if not using the CLI?

@DavidStirling
Copy link
Member

While cmd.parseArgs() works well as a resetter in the bioformats2raw PR, trying to call it with raw2ometiff is generating the following error:

picocli.CommandLine$MissingParameterException: Missing required parameters: '<inputPath>', '<outputPath>'
	at picocli.CommandLine$Interpreter.assertNoMissingParameters(CommandLine.java:14918)
	at picocli.CommandLine$Interpreter.validateConstraints(CommandLine.java:13653)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:13614)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:13559)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:13454)
	at picocli.CommandLine.parseArgs(CommandLine.java:1552)
	at com.glencoesoftware.convert.tasks.CreateTiff.<init>(CreateTiff.java:64)
	at com.glencoesoftware.convert.workflows.ConvertToTiff.<init>(ConvertToTiff.java:25)
	... 43 more

I can't see anything significantly different in the CLI implementation, aside from the underlying i/o variables being Path objects instead of Strings. Not sure if that could be causing it to reject.

@melissalinkert
Copy link
Member Author

@DavidStirling: sorry, the build was failing quietly here due to a merge conflict, so it's likely you're seeing MissingParameterException because the relevant default value changes weren't included. f996b88 fixes the conflict and builds now pass, so see if it works after updating?

Copy link
Member

@DavidStirling DavidStirling left a comment

Choose a reason for hiding this comment

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

Working now, thanks!

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

Thanks @melissalinkert. Looks good to me and happy to move forward with this strategy together with glencoesoftware/bioformats2raw#226 in the upcoming 0.6.0 release of raw2ometiff

Before merging, could we add a simple testResetOptions unit test demonstrating and covering the usage of the API to reset options similarly to https://github.com/glencoesoftware/bioformats2raw/pull/226/files#diff-3bf686322477afd2500c0b42697eb037dd07ae3fc65e17ece2facd25bf5c6081R1997-R2035?

@sbesson sbesson merged commit 2ae6436 into glencoesoftware:master Nov 30, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants