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

Mylin/1617 support spatial profile line polyline #1812

Merged
merged 55 commits into from
May 10, 2022

Conversation

acdo2002
Copy link
Contributor

Fixed #1617
The corresponding backend PR1020

@kswang1029
Copy link
Collaborator

kswang1029 commented Apr 14, 2022

I have had a preview of this branch and found few issues that need to be addressed:

  1. when the region is point or cursor, the x label of the profile plot is "X coordinate" or "Y coordinate" (with an implicit unit of "pixel". When the region is line (not polyline), the x label should be revised to offset with a unit (eg Offset (arcsec). The actual unit (arcsec, arcmin, or degree) is determined by the angular size of the line region and the backend should already have a handle on this based on the implementation of pv generator. When the region is polyline, the x label should be "Distance" with a unit. I suspect we need to update protobuf message SPATIAL_PROFILE_DATA.
  2. If we update the line or polyline region and before the new results come in, we need to have visual indication that the backend is working on it. Similar to region spectral profilers, we should set the old profile dimmer.
  3. the red vertical line should disappear when the region type is line and polyline. (as a future enhancement, we should display a marker along the line or polyline region and sync its movement with the red vertical line in the spatial profile plot)
  4. if the active line region is deleted, the spatial profiler has an odd behavior displaying the new active region.
Screen.Recording.2022-04-14.at.17.59.02.mov
  1. the title of the spatial profiler widget should be more dynamic based on what is being displayed. Before the support of line and polyline region, it is like "X Profile: Cursor". Now this should be like "Profile: region 2" when region 2 is a line region or a polyline region.
  2. some UI elements in the smoothing tab of the settings dialog are not rendered properly.
    Screen Shot 2022-04-14 at 18 04 45

@kswang1029
Copy link
Collaborator

@TienHao as we have a new “compute” tab in the settings dialog, please add a placeholder for the in-app help.

@kswang1029
Copy link
Collaborator

kswang1029 commented Apr 27, 2022

@TienHao Please help to refine the cursor info:

  1. when the region is line or polyline, we don't need to show world coordinate in the cursor info
  2. please switch to scientific notation to show distance or offset. %.5e should be good enough.
  3. Replace "Image" to "Offset" (for line region) or "Distance" (for polyline region).

Screen Shot 2022-04-27 at 11 41 38

@kswang1029
Copy link
Collaborator

kswang1029 commented Apr 27, 2022

@TienHao The tsv header and file name need another tweak. When the profile is from line or polyline region, we should say "Region Profile" instead of "X profile" in the first line of the tsv file. Similar for the file name of the tsv.
Screen Shot 2022-04-27 at 11 55 05

@kswang1029
Copy link
Collaborator

@TienHao please enable auto y-scale when zooming for a line or polyline profile, just like point or cursor region profile.

@kswang1029
Copy link
Collaborator

kswang1029 commented Apr 27, 2022

@TienHao The title of the settings dialog needs a tweak too. I suggest we unify it to "Spatial Profile Settings"

In addition, the show mean/rms function does not work for line/polyline profile.
Screen Shot 2022-04-27 at 13 26 18

* refined cursor info

* modified exportData header and title of settings dialog

* enable auto y-scale zooming

* fixed Mean/RMS
@TienHao TienHao marked this pull request as ready for review April 29, 2022 09:47
@kswang1029 kswang1029 self-requested a review April 29, 2022 09:54
@kswang1029
Copy link
Collaborator

@YuHsuan-Hwang @veggiesaurus I have had brief tests during the draft PR stage. Now it is ready for code review. I will continue my tests before approving.

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.

@TienHao @acdo2002 this works nicely. 👍

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.

The changes look good to me. only very minor issues.

src/stores/Frame/RegionStore.ts Outdated Show resolved Hide resolved
src/stores/Frame/RegionStore.ts Outdated Show resolved Hide resolved
@acdo2002
Copy link
Contributor Author

acdo2002 commented May 6, 2022

@YuHsuan-Hwang I try my best to solve the merge conflicts (as well as solve the minor issue you suggested), could you check whether this branch is working fine or not?
At least this branch can build successfully on my MacOS 11 laptop, I will try to build on my Ubuntu laptop in the weekend.

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.

@YuHsuan-Hwang I try my best to solve the merge conflicts (as well as solve the minor issue you suggested), could you check whether this branch is working fine or not?
At least this branch can build successfully on my MacOS 11 laptop, I will try to build on my Ubuntu laptop in the weekend.

@acdo2002 looks fine to me 👍

@acdo2002
Copy link
Contributor Author

acdo2002 commented May 9, 2022

I have tried to build it on ubuntu 20.04, it looks fine.

@veggiesaurus veggiesaurus merged commit 27a932e into dev May 10, 2022
@veggiesaurus veggiesaurus deleted the mylin/1617-support_spatial_profile_line_polyline branch May 10, 2022 17:03
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.

Expand the features in the spatial profiler: line region/averaging width
5 participants