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

Flags to rename and pyramid.n5 and scaler paths #19

Merged
merged 3 commits into from
Apr 1, 2020

Conversation

perlman
Copy link
Contributor

@perlman perlman commented Mar 19, 2020

neuroglancer's n5 implementation uses "s%d" for each scale.

By using "s%d" instead (as an option) allows this an option. The default remains "%d".

@melissalinkert
Copy link
Member

Thank you for contributing this, @perlman. The changes look fine to me, and increasing compatibility with other tools makes sense.

The only thing I can think of that might be nice is to somehow warn that using using non-default values for these options will break compatibility with https://github.com/glencoesoftware/raw2ometiff (the second half of the workflow this repo was designed for). That could be something in the option descriptions, or a message logged at WARN if non-default values are detected.

@perlman
Copy link
Contributor Author

perlman commented Mar 26, 2020

Your request makes sense -- there is no harm in being verbose here. I'll update with something later this week.

@perlman
Copy link
Contributor Author

perlman commented Mar 31, 2020

I've updated to add the bare minimum warning to the help text ("[Can break compatibility with raw2ometiff]") and a logger.INFO message when the values are changed.

I looked into using a picocli ArgGroup to cluster the flags which might break raw2ometiff support, and possibly use that as hook to see if the options are changed, but that feature was introduced in picocli 4.0 (which is a breaking update from 3.9.6). 🤷‍♂️

@melissalinkert
Copy link
Member

Latest commits look fine, and all works for me in a final test. Merging to get these new commands included.

I don't have a problem with upgrading picocli in a follow-up pull request if needed. We've upgraded from 3.9.3 to 4.1.4+ elsewhere with no issues.

@melissalinkert melissalinkert merged commit f4f7241 into glencoesoftware:master Apr 1, 2020
@joshmoore
Copy link
Contributor

Hi @perlman. Not sure of exactly which repo to discuss on, but do you have any suggestions how we might synchronize rollout of the naming changes from zarr-developers/zarr-specs#50 for neuroglancer, bioformats2raw, etc.?

@perlman
Copy link
Contributor Author

perlman commented Apr 8, 2020

Hi @joshmoore!

I've been following that thread and almost every minor concern I may have had has been addressed in the comments. I'll go through again and see if I have anything to add.

I'm excited by the idea of a synchronized rollout of the new format (and am fully aware that it obliterates my motivation for this specific PR.) I think an issue on each repo, referencing the proposal, would be good place to start?

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

Successfully merging this pull request may close these issues.

3 participants