Add total voxel count and dataset extent to tooltip in dataset info tab #8229
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several enhancements to the WEBKNOSSOS application, including the addition of a tooltip displaying the total volume of datasets in both voxel and physical units. It modifies the handling of remote URIs by ignoring format prefixes, improves asynchronous image file reading, and enhances error messages related to dataset jobs. Bug fixes address performance issues and incorrect segment color updates. Additionally, the integration with Google Analytics has been removed. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
CHANGELOG.unreleased.md (1)
15-15: Enhance the changelog entry with more specific details.The current entry could be more descriptive about the tooltip's functionality. Consider expanding it to explicitly mention both display units and its location.
-Added the total volume of a dataset to a tooltip in the dataset info tab. [#8229](https://github.com/scalableminds/webknossos/pull/8229) +Added a tooltip in the dataset info tab displaying the total volume of a dataset in both voxel count and physical units, shown next to the factorized dataset extent. [#8229](https://github.com/scalableminds/webknossos/pull/8229)frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx (2)
133-136: Fix variable naming conventionThe variable name has inconsistent casing.
- const formattedExtentinUnit = formatNumberToVolume( + const formattedExtentInUnit = formatNumberToVolume(
138-148: Enhance tooltip content formattingConsider improving the tooltip's readability by:
- Adding labels for each value
- Using consistent spacing
const renderDSExtentTooltip = () => { return ( <div> - Dataset extent: + <strong>Dataset Extent</strong> <br /> - {formatVoxels(extentProductInVx)} - <br /> - {formattedExtentinUnit} + Volume: {formatVoxels(extentProductInVx)} + <br /> + Physical Volume: {formattedExtentInUnit} </div> ); };frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts (2)
355-360: Add JSDoc documentation for the new exported functions.The functions should be documented to explain their purpose, parameters, return values, and potential errors.
+/** + * Calculates the total number of voxels in the dataset by multiplying width, height, and depth. + * @param dataset The dataset to calculate the extent for + * @returns The total number of voxels + * @throws {Error} If the product exceeds the maximum safe number + */ export function getDatasetExtentInVoxelAsProduct(dataset: APIDataset) { return getDataSetExtentAsProduct(getDatasetExtentInVoxel(dataset)); } +/** + * Calculates the total volume of the dataset in physical units by multiplying width, height, and depth. + * @param dataset The dataset to calculate the extent for + * @returns The total volume in dataset units + * @throws {Error} If the product exceeds the maximum safe number + */ export function getDatasetExtentInUnitAsProduct(dataset: APIDataset) { return getDataSetExtentAsProduct(getDatasetExtentInUnit(dataset)); }
355-360: Consider memoizing the extent product calculations.Since these functions might be called frequently (e.g., in UI renders) and datasets are typically immutable, consider memoizing the results using the existing memoization pattern from the codebase.
+const _getDatasetExtentInVoxelAsProduct = (dataset: APIDataset) => { + return getDataSetExtentAsProduct(getDatasetExtentInVoxel(dataset)); +}; + +export const getDatasetExtentInVoxelAsProduct = memoizeOne(_getDatasetExtentInVoxelAsProduct); + +const _getDatasetExtentInUnitAsProduct = (dataset: APIDataset) => { + return getDataSetExtentAsProduct(getDatasetExtentInUnit(dataset)); +}; + +export const getDatasetExtentInUnitAsProduct = memoizeOne(_getDatasetExtentInUnitAsProduct);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx(2 hunks)
🔇 Additional comments (3)
frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx (2)
9-14: LGTM: Import changes align with new functionality
The new imports support the addition of volume calculations and unit conversions for the dataset extent tooltip.
131-151: LGTM: Dataset extent tooltip implementation
The implementation successfully:
- Calculates total volume in both voxels and physical units
- Provides clear tooltip information
- Uses proper unit conversion
Let's verify the usage of the formatting utilities:
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient information about the formatting utilities. The test cases in format_utils.spec.ts show comprehensive coverage of various scenarios:
Verified: Dataset extent tooltip uses robust formatting utilities
The implementation is correct and robust because:
formatNumberToVolumehandles a wide range of volumes from very small (1e-33 nm³) to very large (1234e33 m³)- Unit conversions are properly tested (mm³ to cm³, µm³ to cm³, etc.)
formatVoxelshas safeguards for large numbers (>2^50) using PVx notation- Both functions are well-tested with edge cases and maintain consistent precision
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of formatNumberToVolume and formatVoxels
# to ensure they handle large numbers appropriately
# Search for test cases of these formatting functions
rg -A 5 "formatNumberToVolume|formatVoxels" --type ts
Length of output: 12109
frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts (1)
355-360: LGTM! Clean integration with existing extent-related functions.
The new functions properly build upon existing functionality and maintain consistency with the codebase.
frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx (2)
131-151: LGTM with minor suggestions for tooltip contentThe implementation correctly calculates and displays both voxel and unit volumes. Consider enhancing the tooltip content to be more descriptive:
- Dataset extent: + Total Dataset Volume: <br /> - {extentProductInVx != null ? formatVoxels(extentProductInVx) : "Unable to calculate"} + Voxels: {extentProductInVx != null ? formatVoxels(extentProductInVx) : "Unable to calculate"} <br /> - {formattedExtentinUnit} + Physical Volume: {formattedExtentinUnit}
133-139: Consider enhancing number formatting for better readabilityFor large datasets, the volume numbers could become quite large. Consider adding options for scientific notation or appropriate number grouping to improve readability.
Example implementation:
const formattedExtentinUnit = extentProductInUnit != null ? formatNumberToVolume( extentProductInUnit, LongUnitToShortUnitMap[dataset.dataSource.scale.unit], + { useScientificNotation: extentProductInUnit > 1e6 } ) : "Unable to calculate";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts
🔇 Additional comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx (1)
9-14: LGTM: Import statements are well-organized
The new imports are properly structured and align with the PR's objective of adding volume calculations to the tooltip.
frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
CHANGELOG.unreleased.md (1)
15-15: Enhance the changelog entry to better reflect the implemented features.The current entry could be more specific about the tooltip's functionality. Consider revising it to explicitly mention both the voxel count and physical unit representation.
- Added the total volume of a dataset to a tooltip in the dataset info tab. [#8229](https://github.com/scalableminds/webknossos/pull/8229) + Added a tooltip in the dataset info tab displaying the total dataset volume in both voxel count and physical units. [#8229](https://github.com/scalableminds/webknossos/pull/8229)frontend/javascripts/libs/format_utils.ts (1)
Line range hint
535-535: Fix inconsistent unit suffix for small numbers.The function returns "B" for small numbers while using "Vx" (voxels) as the unit suffix for larger numbers. This inconsistency could confuse users.
Apply this diff to fix the inconsistency:
- return `${voxelCount} B`; + return `${voxelCount} Vx`;frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts (1)
349-355: Consider handling numeric overflow for large datasets.The multiplication of dimensions could potentially overflow for large datasets. Consider using safe multiplication or handling overflow cases.
function getDatasetExtentAsProduct(extent: { width: number; height: number; depth: number; }) { - return extent.width * extent.height * extent.depth; + const result = extent.width * extent.height * extent.depth; + if (!Number.isFinite(result)) { + // For very large datasets, return Infinity instead of NaN + return Number.POSITIVE_INFINITY; + } + return result; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/libs/format_utils.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx
🧰 Additional context used
📓 Learnings (1)
frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8229
File: frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts:348-354
Timestamp: 2024-11-25T14:38:49.345Z
Learning: For the `getDatasetExtentAsProduct` function in `dataset_accessor.ts`, input validation for negative or zero dimensions is not necessary.
🔇 Additional comments (7)
CHANGELOG.unreleased.md (1)
Line range hint 1-24: LGTM! Well-structured changelog following best practices.
The changelog is well-organized, with appropriate categorization of changes and consistent formatting throughout. Each entry includes a PR reference, making it easy to track the source of changes.
frontend/javascripts/libs/format_utils.ts (4)
516-521: LGTM! Improved edge case handling.
The separation of null and non-finite checks makes the code more maintainable and explicit.
Line range hint 522-534: LGTM! Well-implemented large number formatting.
The implementation:
- Uses appropriate binary prefixes (K, M, G, T, P)
- Maintains consistent precision using toPrecision(4)
- Follows the established pattern used in other formatting functions
Line range hint 516-535: Verify function usage and test coverage.
Let's verify the usage of this function and ensure test coverage for the new edge cases.
✅ Verification successful
Based on the shell script results, I can now provide a final response:
The formatVoxels function is properly used and tested in the codebase.
The function is used in two specific locations:
frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx: For displaying the total voxel count in the dataset info tabfrontend/javascripts/oxalis/view/jobs/train_ai_model.tsx: For showing bounding box voxel counts in the AI model training view
The implementation follows the same pattern as other similar formatting functions in the file (like formatBytes), and the usage is consistent with the PR's objective of displaying voxel counts in a human-readable format.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of formatVoxels function and related test files
# Find all files that import or use formatVoxels
echo "Files using formatVoxels:"
rg "formatVoxels" --type ts --type tsx
# Look for related test files
echo -e "\nRelated test files:"
fd "format.*test\.(ts|tsx)$"
# Check for existing test cases in test files
echo -e "\nExisting test cases:"
rg "test|describe.*formatVoxels" --type ts --type tsx
Length of output: 354
Script:
#!/bin/bash
# Let's try a different approach with more specific commands
# Search for formatVoxels usage with ripgrep
echo "Files using formatVoxels:"
rg "formatVoxels" -t typescript
# Find test files related to format utils
echo -e "\nFormat util test files:"
find . -type f -name "format*test.ts" -o -name "format*test.tsx"
# Check content of format utils test file
echo -e "\nTest file content:"
find . -type f -name "format*test.ts" -o -name "format*test.tsx" -exec cat {} \;
# Look for any imports of format_utils
echo -e "\nFiles importing from format_utils:"
rg "from.*format_utils" -t typescript
Length of output: 556
Script:
#!/bin/bash
# Let's try without file type specifications and use simpler commands
# Search for formatVoxels usage
echo "Files using formatVoxels:"
rg "formatVoxels"
# Find test files
echo -e "\nFormat util test files:"
rg "format_utils.*test" --files-with-matches
# Look for imports
echo -e "\nFiles importing format_utils:"
rg "import.*format_utils"
# Check the content of the format_utils.ts file
echo -e "\nFormat utils implementation:"
cat frontend/javascripts/libs/format_utils.ts
Length of output: 21417
Line range hint 516-535: Verify physical unit display implementation.
The PR objective mentions displaying both voxel count and physical unit in the tooltip. While this function handles the voxel count formatting, let's verify the physical unit display implementation.
✅ Verification successful
Physical unit display is properly implemented.
Looking at the DatasetExtentRow component in dataset_info_tab_view.tsx, both voxel count and physical unit are displayed in the tooltip. The component uses formatVoxels for voxel count and formatNumberToVolume with the appropriate unit for physical size. This is exactly what was intended in the PR objective.
Example from the code:
const renderDSExtentTooltip = () => {
return (
<div>
Dataset extent:
<br />
{formatVoxels(extentProductInVx)}
<br />
{formattedExtentinUnit}
</div>
);
};🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for code handling physical unit display in the dataset info tab
# Search for dataset info tab component
echo "Dataset info tab implementation:"
fd "dataset.*info.*tab" --type ts --type tsx
# Look for tooltip implementations
echo -e "\nTooltip implementations:"
rg "tooltip.*dataset.*extent|dataset.*extent.*tooltip" --type ts --type tsx
Length of output: 462
Script:
#!/bin/bash
# Search for dataset info tab component with correct file extensions
echo "Dataset info tab implementation:"
fd "dataset.*info.*tab" -e ts -e tsx
# Look for tooltip implementations without file type restrictions
echo -e "\nTooltip and extent related implementations:"
rg "tooltip.*dataset.*extent|dataset.*extent.*tooltip|extent.*tooltip"
# Search for physical unit related code
echo -e "\nPhysical unit related code:"
rg "physical.*unit|unit.*display|extent.*unit"
# Look for other tooltip implementations in the same directory
echo -e "\nTooltip implementations in format_utils and nearby files:"
rg "tooltip" frontend/javascripts/libs/
Length of output: 1119
Script:
#!/bin/bash
# Let's examine the dataset info tab view implementation
echo "Dataset info tab view content:"
cat frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx
# Look for voxel size and extent related code
echo -e "\nVoxel size and extent related code in format_utils:"
rg -A 5 "voxel.*size|extent" frontend/javascripts/libs/format_utils.ts
# Check for other formatting functions in format_utils
echo -e "\nOther formatting functions in format_utils:"
ast-grep --pattern 'function $NAME($$$) {
$$$
}' frontend/javascripts/libs/format_utils.ts
Length of output: 26032
frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts (2)
356-358: LGTM! Clean and focused implementation.
The function correctly uses the existing getDatasetExtentInVoxel function to calculate the total voxel count.
359-361: LGTM! Clean and focused implementation.
The function correctly uses the existing getDatasetExtentInUnit function to calculate the total volume in physical units.
frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx (1)
139-149: Enhance tooltip accessibility and semanticsConsider these improvements for better accessibility and semantic structure:
const renderDSExtentTooltip = () => { return ( - <div> + <div role="tooltip" aria-label="Dataset extent information"> - Dataset extent: - <br /> - {formatVoxels(extentProductInVx)} - <br /> - {formattedExtentInUnit} + <strong>Dataset extent:</strong> + <dl> + <dt>Voxels:</dt> + <dd>{formatVoxels(extentProductInVx)}</dd> + <dt>Volume:</dt> + <dd>{formattedExtentInUnit}</dd> + </dl> </div> ); };This change:
- Adds ARIA attributes for better screen reader support
- Uses semantic HTML elements (
dl,dt,dd) for better structure- Improves visual hierarchy with
strongelement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🔇 Additional comments (2)
frontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx (2)
9-14: LGTM! Import changes are well-organized
The new imports are properly structured and align with the feature requirements for calculating and displaying dataset volumes.
132-137: LGTM! Volume calculations are properly implemented
The implementation correctly calculates and formats the dataset extent in both voxels and physical units, aligning with the PR objectives.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)