Firefly-75: Implement new WCS matching scheme #825
Conversation
|
Implementation is based on this document: https://confluence.ipac.caltech.edu/display/FIREFLY/WCS+Match+-+Lock+-+Align+issues Some comments on the implementation: Number 12: I am not inclined to implement number 12. There seems to be a lot of undefined aspects to it. In general trying to make image WCS match to a HiPS is a mess. Usually they will disappear for on reason or the other. That why I have avoided it in the past. Even the idea of restoring zoom on a image with disabling the matching does not make sense since it breaks the way everything works. Popup Messages (from number 4 and 12): In general I don't think a pop up message is a good idea when they can happen every time a button is pushed. I image a case when as use click this on several times and keep having to acknowledge this message. There are other ways we could indicate an image does not have a wcs. It could be in the mouse readout or on the title bar of the image. Align by Pixel at Image Center: The "Align by Pixel" went smooth so it will be in the pull request. I don't like it that much so I added a 4th option "Align by Pixel at Image Center". You can give me feed back. If no one likes it then I will take it out. |
robyww
force-pushed
the
firefly-75-wcsmatch
branch
from
7330946 to
0bdaa64
Compare
| @@ -839,5 +840,5 @@ function ratioSize(oldSize, newSize, cc, toSize) { | |||
| } | |||
|
|
|||
| export function hasNoProjection(cc) { | |||
| return !cc.projection.isSpecified()||!cc.projection.isImplemented(); | |||
| return !hasWCSProjection(cc); | |||
There was a problem hiding this comment.
Is there a difference between Projection vs WCSProjection in the future? If not, then you should remove this function. If so, then you should put them next to each other and describe the differences.
| * @param {string} [p.hipsUrlRoot] | ||
| * @param {CoordinateSys} [p.coordSys] | ||
| * @param {WorldPt} [p.centerProjPt] | ||
| * @param {boolean} [p.applyToGroup], apply to the whole group it is locked |
There was a problem hiding this comment.
Extra , in [p.applyToGroup],
| @@ -135,6 +123,7 @@ MultiViewStandardToolbar.propTypes= { | |||
| viewerPlotIds : PropTypes.arrayOf(PropTypes.string).isRequired, | |||
| handleInlineTools : PropTypes.bool, | |||
| alwaysShowToolbar : PropTypes.bool, | |||
| makeDropDownFunc: PropTypes.func | |||
| makeDropDownFunc: PropTypes.func, | |||
| makeDropDown: PropTypes.bool | |||
There was a problem hiding this comment.
You should double check this. makeDropDown is defined as bool but used as a function at line 90. Also, makeDropDownFunc is not referenced in this file.
There was a problem hiding this comment.
That's a big change (apart from the code changes) - i just went through very quickly but i'm afraid we will need Gregory/Vandana to make their own tests.
I'm testing it. The icon and menu are all good.
I have tested aligned and lock and looks good.
I have noticed something (issue?) though:
if i request WCS match/lock aligned, then all images aligned accordingly. Now, if ask 'North-up' , it will do the north-up and rotate all image accordingly. So far so good.
if unlock them and then if i click north-up back again, even though i'm on 'unlocked' mode, all the other images rotate back to original orientation too. I was expecting only the image highlighted to change, isn't it?
|
I will look into that. Now that there are two locks, north up might be checking the wrong one. |
|
(comments duplicated in FIREFLY-75 with colors because cut-and-paste removed them.) Testing at https://irsawebdev9.ipac.caltech.edu/firefly-75-wcsmatch/firefly/: Related tickets: IRSA-1540 - fixed IRSA-1543 - still a problem IRSA-1544 - fixed IRSA-1660 - fixed IRSA-1727 - fixed Proposed Rules:
a. The ticket calls for a new alignment / lock menu. On the confluence page, the icon emphasizes the alignment. In the implementation, the icon emphasizes the lock. Haven't thought a lot about this, yet, but flagging for more eyes. b. The implementation has an additional alignment options that I don't see in the ticket: "Align by Pixel at Image Center" and "Align by Pixel at Image Center & Lock". c. The menu has the option "Unlocked" instead of "Unlock". The latter was requested, and is more in line with the other verbs in the menu. 7. If the selected image is a HiPS image, the "Align by Pixel" option should be greyed out. The "Align by Pixel" option is greyed out, but the "Align by Pixel & Lock" option is still available. This should also be greyed out. 10. If there's only a single image loaded (whether FITS or HiPS), disable "Align by WCS", "Align by Pixel", and "Align by Target", but not the other four options. This looks correct. 21. When starting an empty session, the state of the Alignment Lock should be configurable. In IRSA Viewer, it should be off by default. This looks correct. 22. The existing lock icon in the image toolbar should no longer affect how images pan, zoom, or rotate. It should only only affect the color table, stretch, and the same overlays that it currently does. The mouseover for this lock icon still says "lock images of all bands for zooming, scrolling, etc." |
|
Issues: Panning when locked doesn't work as expected. If I load a HiPS image and a FITS image, click on the FITS image, "Align by WCS & Lock", and then click on the HiPS image and drag it, the FITS image doesn't move as I would have expected. The images are still locked according to the icon, but they aren't locked in practice. 4a. The effect of "Align by WCS" is to make the center of the display sample the same coordinate for all images; to have the same ratio of display pixels to angles on the sky, evaluated at the center of the display; and to align the coordinate axes at the centers of the displays. This seems to work fine. 4b. If any non-selected images are HiPS, they may not be able to be rotated to align coordinate axes with the selected image. If the selected image is a HiPS map, any other HiPS maps will have their displayed coordinate systems changed to match the selected HiPS map (i.e., the equatorial vs. Galactic choice is made uniform for all HiPS maps). The "Options: Galactic" for unselected HiPS maps toggles to match the selected HiPS map even when I unlock the images. 4c. Any non-selected images without a WCS will not be moved. A dialog will pop up to say "Not all images have a valid WCS." and you can then dismiss it with an "OK" button. I tried uploading a DSS image that @ejoliet says doesn't have a valid WCS: https://irsadev.ipac.caltech.edu/data/DSS/images/dss2ir/dss2ir_XI175.fits. It does move when I WCS match it with another FITS image that is clearly centered on a different sky coordinate. However, if I select this FITS image and match HiPS maps to it, and then zoom in on the FITS image, the HiPS maps don't zoom with the FITS image. Maybe that has to do with the WCS?
I tried uploading a DSS image that @ejoliet says doesn't have a valid WCS: https://irsadev.ipac.caltech.edu/data/DSS/images/dss2ir/dss2ir_XI175.fits. The "Align by WCS" option is not greyed out. 6a. The effect of "Align by Pixel" is to match the displayed position of the pixel with coordinates (0,0), whether or not that pixel is within the viewing window; each image pixel will take up the same number of display pixels; and the pixel coordinate axes will be aligned. I tried an AllWISE and SDSS image centered roughly on Arp220 (different sizes). Align by Pixel worked well, but not if I did "North Up" first. Then something strange happened. 6b. If any non-selected images are HiPS, and if the selected image has a WCS, the HiPS images will pan and zoom as if "Align by WCS" had been selected. If I align to an SDSS image, this didn't align properly unless I did "North Up" first. Needs Better Testing: What happens for an image without a WCS. ** |
|
Proposed Rules, Cont'd: 8a. The effect of "Align by Target" is to put the target (if any) associated with each image in the same relative position in the frame; to have the same ratio of display pixels to angles on the sky, evaluated at the target; and to align the coordinate axes at the target. I did an AllWISE w1 and SDSS r band search on Arp 220 with no cutout size. Firefly told me that SDSS didn't have anything there. I added in a cutout size of 500 arcsec, and the SDSS cutout was retrieved fine. I was able to reproduce this. This should probably be made into another ticket if it is verified. In any case, "Align by Target" worked fine. 8b. Non-selected images without targets should pan, zoom, and rotate as if "Align by WCS" had been selected. This seemed to work.
The 2MASS Full J and K-band images didn't seem to WCS match properly. I tried to test this by WCS matching in single image mode and then going to grid mode. It didn't work as I expected. Changing viewing modes changed the selected image zoom. I also tried turning off one of the images in the list, WCS matching, and turning the image back on. If you turn the selected image off, then it picks another image to be the selected one, but also changes the zoom on that image. |
|
Issues, cont'd
|
|
Proposed Rules, Cont'd:
Seems to work.
Works.
Not implemented. We should discuss. 14-19. Pan to Catalog Not requested for this ticket. 1, 13, 20. Table Headers, nothing to implement. |
robyww
force-pushed
the
firefly-75-wcsmatch
branch
from
5634f95 to
0e4f6c5
Compare
|
I am pushed a commit with some fixes. Here are responses to the following issues. I am sure I have missed some. The next step is probably identify exactly what is left to do. Updated: https://irsawebdev9.ipac.caltech.edu/firefly-75-wcsmatch/firefly/
fixed
updated icons - probably still need work
fixed
fixed
fixed
fixed, note - this use case shows how nice "align by pixel at image center" is
No sure what the correct thing to do here is.
I am not seeing issue here. Probably need more details to reproduce.
Resizing to fit or fill when the visible image count changes has been in firefly for a long time and has a lot of uses. I think if we want to change this behavior we should discuss all the details and the use cases and pursuit it in a separate ticket. |
robyww
force-pushed
the
firefly-75-wcsmatch
branch
from
5d28757 to
baea1f6
Compare
|
As requested, I tried loading an image without WCS. I loaded in a bunch of M8 images from IRSA (WISE, SEIP, and MSX, because it’s in gal coords not RA/Dec). I was screwing around and testing various things. Then, I got one of the newly released V838Mon Herschel images, because I remembered Justin saying that the WCS isn’t right. Welp, it well and truly killed my session. Like, the whole browser window is grey, no images shown at all, locked or otherwise. I restarted the tool and it seems to work until I pick “align by WCS” and then it dies - the whole browser window is grey. “Align by target and lock” behaves strangely; see screen shot (“one.png") in Jira ticket. V838 Mon is in a totally different region of the sky than M8 (which is undoubtedly at least part of why "align by WCS” fails so dramatically), and I’ve aligned by target, but the V838 image isn’t centered on the target. Maybe it’s centered on the 0,0 px which is the lower right of the image? I guess I didn’t expect it to not center in the middle of the image. |
robyww
force-pushed
the
firefly-75-wcsmatch
branch
from
baea1f6 to
5f37efd
Compare
- WCS, Target, unlock match working from dropdown - refactored locking to break up overlay/color and position - HiPS working - pixel matching working - pixel center matching - multiple icons - fixed no WCS bug - cleaned out old UI - WCS match type is a startup option - fixed rotation issue - fix overlay lock tip - Gray out align by pixel/lock on hips - fixed: Look at hips gal/j2000 switching - fixed: north up or other rotates not correct when aligned by pixel - improved icons - update the HiPS matching on scroll move - revamp the dropdown from original idea
robyww
force-pushed
the
firefly-75-wcsmatch
branch
from
5f37efd to
42e452a
Compare
Firefly-75: Implement new WCS matching scheme
To test:
https://irsawebdev9.ipac.caltech.edu/firefly-75-wcsmatch/firefly/
You are welcome to give feedback on the icons but remember we only have limited skill to create new icons the match the current style.