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

feat(chartViewport): segmentation :: 4D chart viewport #3616

Merged

Conversation

lscoder
Copy link
Collaborator

@lscoder lscoder commented Aug 24, 2023

No description provided.

@lscoder lscoder requested a review from sedghi August 24, 2023 13:36
@lscoder lscoder self-assigned this Aug 24, 2023
package.json Outdated
@@ -88,6 +88,7 @@
"copy-webpack-plugin": "^9.0.1",
"cross-env": "^5.2.0",
"css-loader": "^3.2.0",
"d3": "^7.8.5",
Copy link
Member

Choose a reason for hiding this comment

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

do we really need all of d3?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We actually need 7 out of 30 packages. I will add only the required ones.

commandsManager: this._commandsManager,
};

previousWorkflowStep?.onBeforeInactivate?.(appContext);
Copy link
Member

Choose a reason for hiding this comment

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

onBeforeInactivate is a bit odd, don't have suggestions though

Comment on lines 310 to 322
function _updateSegmentationsDisplaySets(appContext) {
const { servicesManager } = appContext;
const { segmentationService } = servicesManager.services;

const segmentations = segmentationService.getSegmentations();
const { seriesMetadata, instance } = _getInstanceFromSegmentations(
segmentations
);

// An event is triggered after adding the instance and the displaySet is created
DicomMetadataStore.addSeriesMetadata([seriesMetadata], true);
DicomMetadataStore.addInstances([instance], true);
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised this exists here, why the sopHandler for the line chart should care about segmentation?

width: widthProp,
height: heightProp,
axis,
series,
Copy link
Member

Choose a reason for hiding this comment

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

I'm surpised there is a prop called series here... is this DICOM series?

Copy link
Collaborator Author

@lscoder lscoder Aug 31, 2023

Choose a reason for hiding this comment

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

No, chart series similar to what you can find in lineChart.stories.mdx


const LineChartViewport = ({ displaySets }) => {
const displaySet = displaySets[0];
const { axis: chartAxis, series: chartSeries } = displaySet.chartData;
Copy link
Member

Choose a reason for hiding this comment

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

again not sure if series is DICOM series or what

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

chartData does not contain any DICOM related data but only label and points.

Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

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

see my comments please

@lscoder
Copy link
Collaborator Author

lscoder commented Aug 31, 2023

Commit 38aab51
User is now able to make changes to segmentations and see the chart updated after moving back to Kinetic Analysis.

@lscoder
Copy link
Collaborator Author

lscoder commented Aug 31, 2023

Commit 294bcfa
Changes x-axis unit when more than 50% of the time points are greater than the divisors (1000 ms to s, 60 s to m, 60 m to h) making it easier to analyze the data

Before
Screenshot_20230831_132921

After
Screenshot_20230831_132747

@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Merging #3616 (ba70453) into feat/preclinical-4d-base (2a5981a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                    Coverage Diff                    @@
##           feat/preclinical-4d-base    #3616   +/-   ##
=========================================================
  Coverage                     42.75%   42.75%           
=========================================================
  Files                            82       82           
  Lines                          1450     1450           
  Branches                        338      338           
=========================================================
  Hits                            620      620           
  Misses                          667      667           
  Partials                        163      163           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a5981a...ba70453. Read the comment docs.

Comment on lines +36 to +42
"d3-array": "3",
"d3-axis": "3",
"d3-scale": "4",
"d3-scale-chromatic": "3",
"d3-selection": "3",
"d3-shape": "3",
"d3-zoom": "3",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copied from D3/package.js

@sedghi sedghi merged commit 3e58deb into OHIF:feat/preclinical-4d-base Sep 6, 2023
3 of 5 checks passed
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.

2 participants