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

PV preview feature #1253

Merged
merged 81 commits into from
May 3, 2023
Merged

PV preview feature #1253

merged 81 commits into from
May 3, 2023

Conversation

pford
Copy link
Collaborator

@pford pford commented Apr 13, 2023

Implements fast PV image preview for #795 .

  • How does this PR solve the issue? Give a brief summary.

    • Handles new messages and submessages added for PV preview (see protobuf PR).
    • Moves code from RegionHandler to new class LineBoxRegions to approximate a line with width as a series of box regions (common to line spatial profiles, PV generator, and PV preview).
    • Preview settings are saved in two new classes, PvPreviewCut and PvPreviewCube:
      • PvPreviewCut holds the settings related to the PV cut and other per-preview settings (width and reverse) needed for updates. When a preview is requested or the PV cut changes in the source image, its region parameters (RegionState) are added to a queue so that they are all processed to "animate" the preview image. PvPreviewCut also compresses the preview image data according to the compression settings. There is one PvPreviewCut per preview.
      • PvPreviewCube stores the parameters for the downsampled image cube and uses them to create a casacore::SubImage or casacore::RebinImage. This smaller image is used to set the line region and generate the PV image for headers. The cube data is loaded from SubImage, rebinned if requested (to improve performance over RebinImage implementation), then stored in memory. This cube data is used to create the profiles which make up the PV preview image data. PvPreviewCubes may be shared among more than one preview.
    • The existing PvGenerator is also used for the preview. It uses the coordinate system from the casacore rebinned SubImage image stored in PvPreviewCube to create the PV preview image (with no data, since it has already been stored in an array and compressed).
  • Are there any companion PRs (frontend, protobuf)?
    protobuf #82 and frontend #2100

  • Is there anything else that testers should know (e.g. exactly how to reproduce the issue)?
    Launch the PV generator widget, set parameters, and click "Start Preview".

Checklist

  • changelog updated / no changelog update needed
  • e2e test passing / added corresponding fix
  • protobuf updated to the latest dev commit / no protobuf update needed
  • added reviewers and assignee
  • added ZenHub estimate, milestone, and release

* Check for valid spectral coordinate in source image
* Pass RegionState for pv cut to update pv prevew
* Move MVDirection mutex to one place in LineBoxRegions
* Pass PvPreviewCut and PvPreviewCube instead of each param
Move computed stokes code to new files
@pford pford marked this pull request as ready for review April 27, 2023 14:18
@pford pford linked an issue Apr 27, 2023 that may be closed by this pull request
@kswang1029
Copy link
Contributor

@confluence @markccchiang please proceed code review for the beta release. I will perform some other tests at the same time.

@acdo2002
Copy link
Contributor

acdo2002 commented May 2, 2023

I tested this branch to current ICD-RxJS, in order to check whether there is any regression.
And I found the PV_GENERATOR_HDF5_COMPARED_FITS.test.ts failed on this branch.
It means the PV generator of fits and hdf5 (the same image but different formats) are not consistent.
For example, I used: HD163296_CO_2_1.fits and HD163296_CO_2_1.hdf5
I generate a region:

setRegion:
{
fileId: 0,
regionId: -1,
regionInfo: {
regionType: CARTA.RegionType.LINE,
controlPoints: [{ x: 79, y: 77 }, { x: 362, y: 360 }],
rotation: 135
}
},

and a PV cut:

setPVRequest:
{
fileId:0,
regionId:1,
width:3,
},

The current dev two PV image (generated by fits and hdf5) subtracted are all 0
Screen Shot 2023-05-02 at 3 17 09 PM

However in this branch, two PV image subtracted are NOT all 0, indicating two PV generated images are not the same.
Screen Shot 2023-05-02 at 3 17 21 PM

Because the code freeze is coming, not sure should fix this issue before the code freeze, or I can block related test and wait later to fix this issue.

@kswang1029
Copy link
Contributor

I tested this branch to current ICD-RxJS, in order to check whether there is any regression. And I found the PV_GENERATOR_HDF5_COMPARED_FITS.test.ts failed on this branch. It means the PV generator of fits and hdf5 (the same image but different formats) are not consistent. For example, I used: HD163296_CO_2_1.fits and HD163296_CO_2_1.hdf5 I generate a region:

setRegion:
{
fileId: 0,
regionId: -1,
regionInfo: {
regionType: CARTA.RegionType.LINE,
controlPoints: [{ x: 79, y: 77 }, { x: 362, y: 360 }],
rotation: 135
}
},

and a PV cut:

setPVRequest:
{
fileId:0,
regionId:1,
width:3,
},

The current dev two PV image (generated by fits and hdf5) subtracted are all 0 Screen Shot 2023-05-02 at 3 17 09 PM

However in this branch, two PV image subtracted are NOT all 0, indicating two PV generated images are not the same. Screen Shot 2023-05-02 at 3 17 21 PM

Because the code freeze is coming, not sure should fix this issue before the code freeze, or I can block related test and wait later to fix this issue.

@acdo2002 please file an issue in the backend repo. This can be addressed after the beta release.

@kswang1029
Copy link
Contributor

@acdo2002 if you have the two images matched, do you see identical region spectral profiles?

@acdo2002
Copy link
Contributor

acdo2002 commented May 2, 2023

@kswang1029 Ok, I will file an issue in the backend repo, and block one sub-test about this test in PV_GENERATOR_HDF5_COMPARED_FITS.test.ts.

Because the Z profile currently is not allow to select the line as region (if set rectangle as the region, the Z profile are the same), I only check the Profile of "SPATIAL_PROFILE_DATA" of the line, two matched images are the same (check by eye as well as rawValuesFp32).

@kswang1029
Copy link
Contributor

kswang1029 commented May 3, 2023

@confluence would you be able to fix the merge conflict in the changelog and protobuf for Pam?

@confluence
Copy link
Collaborator

@confluence would you be able to fix the merge conflict in the changelog and protobuf for Pam?

Yes, it shouldn't be a problem.

@github-actions
Copy link

github-actions bot commented May 3, 2023

Code Coverage

Package Line Rate Health
src.Cache 65%
src.DataStream 52%
src.FileList 67%
src.Frame 50%
src.HttpServer 43%
src.ImageData 28%
src.ImageFitter 89%
src.ImageGenerators 44%
src.ImageStats 76%
src.Logger 44%
src.Main 54%
src.Region 18%
src.Session 29%
src.Table 52%
src.ThreadingManager 87%
src.Timer 85%
src.Util 50%
Summary 38% (6873 / 18165)

@confluence confluence merged commit fc54964 into dev May 3, 2023
@confluence confluence deleted the pam/795_pv_preview branch May 3, 2023 11: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
Development

Successfully merging this pull request may close these issues.

support preview of a pv image from a downsampled cube (backend)
5 participants