Thchang/582 improve coordinate system selection when loading images

[TienHao]

Description

This PR closes #582 and fixes a regression of updating defaultSystem(overlayStore) from AST.

This PR makes a improvement for switching coordinate system after loading or appending images, especially when the user has already loaded a image and explicitly specified a coordinate system(not Auto).

• In the loading scenario, the explicitly specified coordinate system:
  changes to Auto.

• In the appending scenario, the explicitly specified coordinate system:
  remains unchanged, if the "WCS matching on append" option in the preference includes "Spatial",
  or changes to Auto for the rest.

This PR also fixes a inconsistency between defaultSystem(overlayStore) and the defined system in the image header. This inconsistency happens after the user explicitly specified a coordinate system. Then if the user loads a new image or changes the active frame, the defaultSystem(overlayStore) will fixed to the specified system.

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
• changelog updated / no changelog update needed
• unit test added (for functions with no dependenies)
• API documentation added (for public variables and methods in stores)

For dependencies:

• e2e test passing / corresponding fix added / new e2e test created
• protobuf version bumped / no protobuf version bumped needed
• protobuf updated to the latest dev commit / no protobuf update needed
• corresponding ICD test fix added (BackendService changed) / no ICD test fix needed (BackendService unchanged)
• user manual prepared (for large new features)

[loveluthien]

@TienHao I am wondering why the issue cannot be solved by resetting the OverlayGlobalSetting to default when loading a new file.

[YuHsuan-Hwang]

In the loading scenario, the explicitly specified coordinate system:

• remains unchanged, if the specified system and the system of the newly loaded image are the same.

Is it simpler if we also set to AUTO here?

In the appending scenario, the explicitly specified coordinate system:

• changes to the system of the spatial reference image, if the "WCS matching on append" option in the preference includes "Spatial".

Should we remain unchanged here?

• remains unchanged, if the "WCS matching on append" option in the preference not includes "Spatial", and the specified system and the system of the newly loaded image are the same.

Is it simpler if we also set to AUTO here?

[TienHao]

@TienHao I am wondering why the issue cannot be solved by resetting the OverlayGlobalSetting to default when loading a new file.

@loveluthien: Yes it can solved like that. I had made this PR too complicated before...(having included a previous comment which needed a check between coordinate system and the system of the loaded image.)

[TienHao]

I have made the switching results of the specified coordinate system just like @YuHsuan-Hwang mentioned above. It’s way simpler now. And the description of PR has been updated.

[YuHsuan-Hwang]

The changes are clear. 👍
Some comments:

[kswang1029]

@TienHao @YuHsuan-Hwang With v4.1/dev branch, I think the issue mentioned in #582 (very old from 2019 so this was referring to the version at that time) does not exist? The coordinate label in the toolbar of the image viewer shows the correct and consistent one. In addition, I think the behavior in dev/v4.1 branch is fine.

In this PR, I see the coordinate system (when it is "WCS") changes with "active" image and affects other images, resulting a non-ideal UX. Ideally, let's try to see if we can achieve the following rules:

1. images not matched to the reference images have independent WCS grid setup.
2. images matched to the reference images share the same WCS grid setup of the reference image. When any of the matched images, including the reference image itself, have a new WCS grid setup, it applies to all matched images and the reference image.

Would this logic be sensible?

[YuHsuan-Hwang]

With v4.1/dev branch, I think the issue mentioned in #582 (very old from 2019 so this was referring to the version at that time) does not exist? The coordinate label in the toolbar of the image viewer shows the correct and consistent one. In addition, I think the behavior in dev/v4.1 branch is fine.

Looks like v4.1 inherit the system instead of resetting to AUTO. Do you prefer we set back to inheriting?
This PR also solved some bugs when setting back to AUTO.

In this PR, I see the coordinate system (when it is "WCS") changes with "active" image and affects other images, resulting a non-ideal UX. Ideally, let's try to see if we can achieve the following rules:

1. images not matched to the reference images have independent WCS grid setup.
2. images matched to the reference images share the same WCS grid setup of the reference image. When any of the matched images, including the reference image itself, have a new WCS grid setup, it applies to all matched images and the reference image.

Would this logic be sensible?

Currently we don't support independent system, which is another issue #1619. We could simply delete some code to retrieve the same behavior in dev, so I would suggest we implement this in another branch.

[kswang1029]

With v4.1/dev branch, I think the issue mentioned in #582 (very old from 2019 so this was referring to the version at that time) does not exist? The coordinate label in the toolbar of the image viewer shows the correct and consistent one. In addition, I think the behavior in dev/v4.1 branch is fine.

Looks like v4.1 inherit the system instead of resetting to AUTO. Do you prefer we set back to inheriting? This PR also solved some bugs when setting back to AUTO.

In this PR, I see the coordinate system (when it is "WCS") changes with "active" image and affects other images, resulting a non-ideal UX. Ideally, let's try to see if we can achieve the following rules:

1. images not matched to the reference images have independent WCS grid setup.
2. images matched to the reference images share the same WCS grid setup of the reference image. When any of the matched images, including the reference image itself, have a new WCS grid setup, it applies to all matched images and the reference image.

Would this logic be sensible?

Currently we don't support independent system, which is another issue #1619. We could simply delete some code to retrieve the same behavior in dev, so I would suggest we implement this in another branch.

@YuHsuan-Hwang @TienHao OK. Let's see if we can try the following logic:

1. the initial "WCS" refers to the initial loaded image
2. if "WCS" is still selected while a new image is appended, WCS refers to the one defined in the appended image (ie we update what it refers to). If "WCS" has been changed to others (ie GAL), then after the appended image is loaded, it is still GAL. So this is a sticky behavior.
3. If there are two images (no matter whether they are matched or not) and WCS is selected, changing the active image won't change what WCS refers to, so that we won't see coordinate system changes when active image is changed.
4. If there are image loaded, and now we load a new one (so images will be closed first), then we reset whatever selected back to WCS and refer WCS to the coordinate system defend in the new image.

Would this be sensible and intuitive to users?

[YuHsuan-Hwang]

1. the initial "WCS" refers to the initial loaded image
2. if "WCS" is still selected while a new image is appended, WCS refers to the one defined in the appended image (ie we update what it refers to). If "WCS" has been changed to others (ie GAL), then after the appended image is loaded, it is still GAL. So this is a sticky behavior.
3. If there are two images (no matter whether they are matched or not) and WCS is selected, changing the active image won't change what WCS refers to, so that we won't see coordinate system changes when active image is changed.
4. If there are image loaded, and now we load a new one (so images will be closed first), then we reset whatever selected back to WCS and refer WCS to the coordinate system defend in the new image.

Would this be sensible and intuitive to users?

Looks good to me.

[kswang1029]

Not sure what we do for exception handling? For example if the appended image has no wcs header defined and if a specific coordinate system (eg GAL) has been selected for the reference image, shall we reset the selection back to WCS and refer it to a sensible default?

Or the case if the reference has no wcs header defined but the appended one has a valid coordinate system?

Update: maybe if we can have an image coordinate (IMG for short) in the selection list, exception won't be an issue as every image should at least support the image coordinate for sure.

[YuHsuan-Hwang]

Not sure what we do for exception handling? For example if the appended image has no wcs header defined and if a specific coordinate system (eg GAL) has been selected for the reference image, shall we reset the selection back to WCS and refer it to a sensible default?

Or the case if the reference has no wcs header defined but the appended one has a valid coordinate system?

Update: maybe if we can have an image coordinate (IMG for short) in the selection list, exception won't be an issue as every image should at least support the image coordinate for sure.

I think we don't have error handling for this. We can file issues with reproduce steps.

[kswang1029]

Not sure what we do for exception handling? For example if the appended image has no wcs header defined and if a specific coordinate system (eg GAL) has been selected for the reference image, shall we reset the selection back to WCS and refer it to a sensible default?
Or the case if the reference has no wcs header defined but the appended one has a valid coordinate system?
Update: maybe if we can have an image coordinate (IMG for short) in the selection list, exception won't be an issue as every image should at least support the image coordinate for sure.

I think we don't have error handling for this. We can file issues with reproduce steps.

OK I filed #2366 for supporting image coordinate grid in the WCS list of the image viewer. No sure if we can handle this new enhancement first then revisit this PR?

[TienHao]

1. the initial "WCS" refers to the initial loaded image
2. if "WCS" is still selected while a new image is appended, WCS refers to the one defined in the appended image (ie we update what it refers to). If "WCS" has been changed to others (ie GAL), then after the appended image is loaded, it is still GAL. So this is a sticky behavior.
3. If there are two images (no matter whether they are matched or not) and WCS is selected, changing the active image won't change what WCS refers to, so that we won't see coordinate system changes when active image is changed.
4. If there are image loaded, and now we load a new one (so images will be closed first), then we reset whatever selected back to WCS and refer WCS to the coordinate system defend in the new image.

Would this be sensible and intuitive to users?

@kswang1029 @YuHsuan-Hwang : I have modified the PR and made the "WCS" refers to the last loaded/appended image. And if the last loaded/appended image is closed, the "WCS" reference will change to the last remained image.

[YuHsuan-Hwang]

Let's set #2366 as a separate issue and proceed with the review.
The new changes look good to me. 👍

[kswang1029]

nice fix! no regression from e2e tests 👍

[kswang1029]

@loveluthien @crocka please help reviewing/testing.

[loveluthien]

Behave like what we expect.

[kswang1029]

looks good with bp5 update.