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

Mark/support loading swapped axes images #2011

Merged
merged 92 commits into from
Mar 22, 2023

Conversation

markccchiang
Copy link
Contributor

@markccchiang markccchiang commented Sep 13, 2022

Description
This is intended to solve #1953, plotting coordinates for swapped-axes image cubes. It has to work with the backend PR 1194. To test it we may need more sample images with swapped-axes cubes.

Checklist

  • changelog updated / no changelog update needed
  • e2e test passing / added corresponding fix
  • protobuf updated to the latest dev commit / no protobuf update needed

… mark/support_loading_swapped_axes_images
… mark/support_loading_swapped_axes_images
… mark/support_loading_swapped_axes_images
@kswang1029
Copy link
Collaborator

kswang1029 commented Mar 3, 2023

Based on my report

Almost the problems are all solved. About PV diagram for the swapped image. Here I have a test, if I set a line region (x coordinate starts from 400.5 and ends to -0.5; y coordinate starts from 20 to ends to 20) in "ngc3079-swap-rfd.fits" and making a PV diagram (e.g. ngc3079-swap-rfd_pv-ConvertToChannel20Image.fits), then the generated PV diagram should be a 2D image in "ngc3079-swap-rdf.fits" of channel of 20 (e.g. ngc3079-swap-rdf-Channel20Image-drop.fits). And the generated PV is consistent to 2D image of channel 20 (two images subtracted and all pixels are 0). This is because the line region length is odd number (if line region length is not odd number then, the generated PV may not 100% match to the 2D image), this part requires more investigation (my feeling is more related to PV rather than supporting swapped image)

I have tested this backend to the current ICD-RxJS, there is one test needs update, once the corresponding backend has been merged, then I can update the test (FILEINFO_FITS_MULTIHDU.test.ts) code very soon.

Please go ahead for the code refactoring and code reviewing.

Again, for the current purpose, we don’t focus on any region related analyses. It is not the scope here, whether the results are correct or not. This is a doc capturing all affected analyses https://docs.google.com/document/d/1eNLZt-VxSrzeYaOFjajJqbKXzIOo-u29xL7g3Cstz2c/edit

@kswang1029
Copy link
Collaborator

@markccchiang when a swapped cube is loaded, we will still need to display wcs: ( , ) in the cursor info bar according to the displayed axes. If it is RA vs freq, then we display something like wcs: (12:30:30, 1400.0), for example. This is to be consistent with what we do for PV images.

Screen Shot 2023-03-07 at 11 46 37

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.

feature-wise this is looking good for the current scope - supporting image visualization of a swapped-axes cube. 👍 Please proceed code review.

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 are looking good! I only have two minor comments.

src/stores/Frame/FrameStore.ts Outdated Show resolved Hide resolved
src/stores/Frame/FrameStore.ts Outdated Show resolved Hide resolved
@YuHsuan-Hwang YuHsuan-Hwang self-requested a review March 13, 2023 02:44
@kswang1029 kswang1029 self-requested a review March 14, 2023 06:39
@kswang1029
Copy link
Collaborator

kswang1029 commented Mar 14, 2023

for some reason e49334d introduces test failures for regions. world coordinates of the region are not updating. There are errors from AST.

Screen Shot 2023-03-14 at 14 50 59

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.

looking good. new e2e tests are implemented.

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.

5 participants