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

fix(MergeDataSource): remove duplicated instances and fix to issue #4349 #4355

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

josue-venegas
Copy link

@josue-venegas josue-venegas commented Sep 2, 2024

Context

Problems:

  1. When having merged two Orthanc (dicomweb) Data Sources it gets the error [Bug] promise.start() is not a function when using merged dataSources #4349, since neither remainingPromises nor requiredSeriesPromises are Promises.
    Capture d’écran du 2024-09-02 16-17-22

  2. Also, when a study is present in both servers, the study's UID is found twice, resulting in duplicate calls to the datasource to fetch the metadata and we get 2 display sets for each series.
    Capture d’écran du 2024-09-02 16-16-02

Changes & Results

  1. Changed promise.start() and p.start() to promise and p, respectively.
  2. Copied the mergeKey behaviour from callToDataSourceAsync to callToDataSource in order to remove duplicate studyUIDs.

Testing

  1. Merge two Orthanc servers using the official documentation. For example:
dataSources: [
    {
      sourceName: 'merge',
      namespace: '@ohif/extension-default.dataSourcesModule.merge',
      configuration: {
        name: 'merge',
        friendlyName: 'Merge dicomweb-1 and dicomweb-2 data at the series level',
        seriesMerge: {
          dataSourceNames: ['dicomweb-1', 'dicomweb-2'],
          defaultDataSourceName: 'dicomweb-1'
        },
      },
    },
    {
      friendlyName: "Orthanc Server",
      namespace: "@ohif/extension-default.dataSourcesModule.dicomweb",
      sourceName: "dicomweb-1",
      configuration: {
        name: "Orthanc",
        wadoUriRoot: "${ORTHANC_ROOT_URI}/wado",
        qidoRoot: "${ORTHANC_ROOT_URI}/dicom-web",
        wadoRoot: "${ORTHANC_ROOT_URI}/dicom-web",
        fullUri: "https://${DEPLOYMENT_HOST}${ORTHANC_ROOT_URI}/dicom-web",
        baseUri: "https://${DEPLOYMENT_HOST}${ORTHANC_ROOT_URI}",
        girderUri: "http://orthanc:8042/dicom-web",
        qidoSupportsIncludeField: false,
        imageRendering: "wadors",
        thumbnailRendering: "wadors",
        requestOptions: {
          ServerToken: (options) => {
            return options.ServerToken;
          },
          ServerURL: (options) => {
            return options.ServerURL;
          },
        },
      },
    },
    {
      friendlyName: "Orthanc External Server",
      namespace: "@ohif/extension-default.dataSourcesModule.dicomweb",
      sourceName: "dicomweb-2",
      configuration: {
        name: "ExternalOrthanc",
        wadoUriRoot: ":81/wado",
        qidoRoot: "http://localhost:81/dicom-web",
        wadoRoot: "http://localhost:81/dicom-web",
        fullUri: "http://localhost:81/dicom-web",
        baseUri: "http://localhost:81",
        girderUri: "http://orthanc:81/dicom-web",
        qidoSupportsIncludeField: false,
        imageRendering: "wadors",
        thumbnailRendering: "wadors",
        requestOptions: {
          ServerToken: (options) => {
            return options.ServerToken;
          },
          ServerURL: (options) => {
            return options.ServerURL;
          },
        },
      },
    },
  ],
  /* defaultDataSourceName: "dicomweb", */
  defaultDataSourceName: "merge",
  1. Upload different instances of the same study on both servers. For example, CT + RTDose in the first server and RTStruct + RTPlan in the second server. You can use this study.

  2. Open OHIF and try to access to that study

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • OS: Fedora 40
  • Node version: v20.12.2
  • Browser: Chrome 127.0.6533.119, Firefox 129.0

Copy link

netlify bot commented Sep 2, 2024

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 02cfc43
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/6712d906452f860008ab346a
😎 Deploy Preview https://deploy-preview-4355--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Sep 2, 2024

Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit 02cfc43
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/6712d906b417510008246a70
😎 Deploy Preview https://deploy-preview-4355--ohif-platform-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sedghi sedghi requested a review from igoroctaviano September 5, 2024 13:14
@igoroctaviano
Copy link
Contributor

@josue-venegas-almonacid have you tried using your dicomweb config with enableStudyLazyLoad true/false?

Copy link
Contributor

@igoroctaviano igoroctaviano left a comment

Choose a reason for hiding this comment

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

The merge does work for me, I'm not sure why its giving you errors. Can you dump the network logs?

Also, I tried your changes and it's failing to load studies using OHIF default config.

@elazarcoh
Copy link

I needed 2 more things to make it work:

  1. fix the tagging part in export const callForAllDataSourcesAsync = async ({
-   const mergedData = data.map((data, i) => tagFunc(data.value, sourceNames[i]));
+  const mergedData = data.map((promiseResult, i) =>
+   promiseResult.status === 'fulfilled' ? tagFunc(promiseResult.value, sourceNames[i]) : []
+  );
  1. add tagging for retrieve.series.metadata
export const mergeMap: MergeMap = {
  'query.studies.search': {
    mergeKey: 'studyInstanceUid',
    tagFunc: x => x,
  },
  'query.series.search': {
    mergeKey: 'seriesInstanceUid',
    tagFunc: (series, sourceName) => {
      series.forEach(series => {
        series.RetrieveAETitle = sourceName;
        DicomMetadataStore.updateSeriesMetadata(series);
      });
      return series;
    },
  },
+  'retrieve.series.metadata': {
+    mergeKey: 'seriesInstanceUid',
+    tagFunc: (series, sourceName) => {
+      Object.values(series).forEach(series => {
+        series.RetrieveAETitle = sourceName;
+        DicomMetadataStore.updateSeriesMetadata(series);
+      });
+      return series;
+    },
+  },
  getStudyInstanceUIDs: {
    mergeKey: 'studyInstanceUid',
    tagFunc: x => x,
  },
};

@sedghi
Copy link
Member

sedghi commented Nov 6, 2024

Please revise this PR by tomorrow to include it in 3.9; otherwise, it will be moved to 3.10-beta. Thanks.

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.

4 participants