vc_render_tifxyz: make voxel metadata flags optional with sane fallback and emit physical OME‑Zarr scales#737
Conversation
giorgioangel
requested review from
SuperOptimizer and
hendrikschilling
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef75af3ff7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| writeZarrAttrs(outFile, vol_path, group_idx, baseZ, slice_step, accum_step, | ||
| accum_type_str, accumOffsets.size(), attrXY, baseZ, CH, CW); | ||
| accum_type_str, accumOffsets.size(), attrXY, baseZ, CH, CW, | ||
| base_voxel_size * ds_scale, voxel_unit); |
There was a problem hiding this comment.
ds_scale is 2^-group_idx (used earlier to map full-resolution coordinates into the chosen downsampled source group), so multiplying base_voxel_size by it inverts the physical spacing: for group_idx > 0, coarser source levels are written with smaller voxel sizes. This makes .zattrs physical metadata wrong by large factors (e.g. group 2 writes 1/4 instead of 4x), which miscalibrates downstream measurement/viewing even when --voxel-size is explicitly provided.
Useful? React with 👍 / 👎.
| for (int l = 0; l <= 5; l++) { | ||
| double s = std::pow(2.0, l); | ||
| const double levelScale = std::pow(2.0, l); | ||
| const double physicalScale = baseVoxelSize * levelScale; |
There was a problem hiding this comment.
This now writes arbitrary physical values into coordinateTransformations.scale, but the repo’s reader in core/src/Volume.cpp:188-196 still requires each dataset scale to be an isotropic power-of-two and throws otherwise. As soon as baseVoxelSize is not exactly a power-of-two (the normal case for real voxel units), rendered outputs become unreadable by code paths that reopen Zarr via Volume::zarrOpen (including VC3D volume loading).
Useful? React with 👍 / 👎.
1 participant
Motivation
--voxel-size/--voxel-unitare truly optional and provide a sensible default when volume metadata is absent or invalid so tools like Neuroglancer get usable physical scale information.Description
--voxel-sizehelp text to document the fallback order (CLI value → volume metadata voxelsize → default1.0).readVolumeVoxelSizeand updated voxel-size resolution invc_render_tifxyz.cppto use--voxel-sizeif present (validated positive), else use metadata when valid, else fall back to1.0while emitting a warning/info message.writeZarrAttrssignature and implementation to acceptbaseVoxelSizeandvoxelUnitand to emitmultiscales.axes[*].unitand per-levelcoordinateTransformations.scaleas physical voxel spacings (files changed:volume-cartographer/apps/src/vc_render_tifxyz.cpp,volume-cartographer/core/include/vc/core/util/Zarr.hpp,volume-cartographer/core/src/Zarr.cpp).std::optionalusage (#include <optional>added).Testing
cmake -S volume-cartographer -B /tmp/vcbuild -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON, which failed due to external dependency fetch being blocked by the environment (git clone ofxtlreturned HTTP 403), so a full build/test could not be completed.gitcommands andnl/sedchecks) to verify option definitions, newreadVolumeVoxelSizehelper, fallback behavior, and that calls towriteZarrAttrsnow pass the computedbase_voxel_size * ds_scaleandvoxel_unit; these checks succeeded..zattrswith a smallvc_render_tifxyzinvocation such asvc_render_tifxyz --pre --zarr-output <out> --volume <vol> --group-idx <g> --voxel-size <value> --voxel-unit <unit>once dependencies are available.Codex Task