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

carli/1758_multiprofile_intensity_unit_conversion #2185

Merged
merged 24 commits into from
Jul 11, 2023

Conversation

crocka
Copy link
Contributor

@crocka crocka commented Jun 6, 2023

Description

The pr resolves #1758 and resolves #1907. It tackles three problems. First, the spectral profiles for different frames when matched should convert to the same intensity unit. Second, spectral matching should be avoided if two images do not have intersecting intensity unit. Third, the intensity unit option should only show the ones that are usable for each of the spectrally matched images.

Originally in the SpectralProfileWidgetStore, the intensityConversion observable is singular, mapping the effectiveFrame to the selected intensity unit. This is undoubtedly incorrect if we have more than one frame. To solve this issue, the intensityConfig object was move to FrameStore, and the intensityConversion is uniquely produced in plotData for each of the frames' profile. To access the frame that correspond to each profile, the frame store of the frame is added to the profile object in the profiles array in SpectralProfileSelectionStore.

Another problem that the pr solves is that the program should avoid spectral matching when the spectrally matched images do not have common intensity unit. This condition is added to the if-statement in setSpectralReference function in FrameStore using the getCommonIntensityOptions function. The getCommonIntensityOptions function searches for the common or intersecting intensity unit with 1) a second frame's intensityConfig, if the arguments is a single IntensityConfig, 2) an array of frame's intensityConfig, if the argument is an array rather than a single instance of IntensityConfig.

The third problem is to allow only the intensity units available for all of the spectrally matched images to show in the intensity option. Knowing that the getCommonIntensityOptions function finds the intersecting intensity unit, the intensityOptions in SpectralProfileWidgetStore simple made use of this function.

Checklist

For linked issues (if there are):

  • assignee and label added
  • ZenHub issue connection, board status, and estimate updated

For the pull request:

  • reviewers and assignee added
  • ZenHub estimate, milestone, and release (if needed) added
  • e2e test passing / corresponding fix added
  • changelog updated / no changelog update needed
  • protobuf updated to the latest dev commit / no protobuf update needed
  • BackendService unchanged / BackendService changed and corresponding ICD test fix added

@crocka crocka added this to the v4.0-stable milestone Jun 6, 2023
@crocka crocka changed the title Carli/1758 multiprofile intensity unit conversion carli/1758_multiprofile_intensity_unit_conversion Jun 6, 2023
Copy link
Collaborator

@YuHsuan-Hwang YuHsuan-Hwang left a comment

Choose a reason for hiding this comment

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

Overall the code looks good, and I have some initial comments. Please also update the description in the PR with the new changes.

src/stores/AppStore/AppStore.ts Outdated Show resolved Hide resolved
src/stores/Frame/FrameStore.ts Outdated Show resolved Hide resolved
@crocka crocka requested a review from YuHsuan-Hwang July 7, 2023 00:56
Copy link
Collaborator

@kswang1029 kswang1029 left a comment

Choose a reason for hiding this comment

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

@crocka If we just load one image and apply intensity unit conversion, it appears that the function is broken. I can see the ylabel of the spectral plot changes accordingly but not the values. Could you have a look?

@kswang1029 kswang1029 self-requested a review July 7, 2023 06:04
Copy link
Collaborator

@kswang1029 kswang1029 left a comment

Choose a reason for hiding this comment

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

looks good now. no regression from e2e tests. 👍

src/stores/Frame/FrameStore.ts Outdated Show resolved Hide resolved
@YuHsuan-Hwang YuHsuan-Hwang merged commit 433a84d into dev Jul 11, 2023
8 checks passed
@YuHsuan-Hwang YuHsuan-Hwang deleted the carli/1758_multiprofile_intensity_unit_conversion branch July 11, 2023 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants