Skip to content

Firefly-1960: Hips Extraction - #2049

Merged
robyww merged 1 commit into
devfrom
FIREFLY-1960-hips-extract
Oct 27, 2026
Merged

robyww merged 1 commit into
devfrom
FIREFLY-1960-hips-extract

Conversation

@robyww

@robyww robyww commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Firefly-1960: Hips Extraction

  • add cube (drill), line, and point extract
  • add cube extract
  • added PlotViewUtil.js and some ExtracxtionUIUtil.js test
  • fixed a bug in rotation checks

Testing

@robyww robyww added this to the 2026.4 milestone Oct 23, 2026
@robyww robyww self-assigned this Oct 23, 2026
@robyww
robyww force-pushed the FIREFLY-1960-hips-extract branch from 9f4296b to 22e1c62 Compare October 23, 2026 18:39
@lrebull

lrebull commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

huh. Well, it seems to work for SPHEREx.
pan stars is a totally different beast. All i get from the extractions are zeros. i never get anythign else, even if i zoom in really far. tried both z and y. am i supposed to get anything other than 0?

@robyww

robyww commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

@lrebull I missed this during testing. I mostly tested the spherex. The extraction is truncating the values to integers. I will see what is going on.

@lrebull

lrebull commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

i agree it is working now!

@robyww
robyww requested a review from kpuriIpac October 23, 2026 21:56

@kpuriIpac kpuriIpac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for adding all of the tests.
I tested the UI, works as expected (aside from one potential bug I've described below). Tried both cube and normal fits.

Few comments:

  • isCsysDirMatching is not called anymore. So its reference be removed from PlotViewUtil-test.js (the jest.spyOn line ~1351) and from isCsysDirMatching
  • very minor: the fields nameOrType and naxis in DirectFileAccess appear to not be in use (claude helped catch this), so can be removed unless left in there for future use.
  • Potential bug:
    • For regular fits (like the PanStarrs example), point and line extraction read tiles at the zoom level currently on screen, this gives different values for different zooms (for the same selected point).
      • For reference, see these 2 tables that i pinned for the same extracted point, just at different zoom levels:
      • Image
      • Image
      • the tile and hdu values are different for the same ra/dec.
  • I say potential bug because I wasn't sure if this is intentional or not.

Other than this, looks good to me!

@robyww

robyww commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor Author

Potential bug:

This is working as intended but I understand why it is concerning. However, that is not really a good solution. The extraction works at the tiles at that level. when you zoom in you get a different set of tiles. If we relied on the lowest level of tiles then line extraction could possibly have to transfer 1500 fits files to work.

@robyww

robyww commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor Author

nameOrType and naxis in DirectFileAccess

they is there for future use

@robyww
robyww force-pushed the FIREFLY-1960-hips-extract branch 3 times, most recently from c11afba to d34c1b8 Compare October 27, 2026 19:41
- add cube (drill), line, and point extract
- add cube extract
- added several test files
- fixed a bug in rotation checks
- response to feedback
- fixed crop
- VisUtil fixes and test
@robyww
robyww force-pushed the FIREFLY-1960-hips-extract branch from d34c1b8 to 988e374 Compare October 27, 2026 20:49
@robyww
robyww merged commit 92f3a1d into dev Oct 27, 2026
2 checks passed
@robyww
robyww deleted the FIREFLY-1960-hips-extract branch October 27, 2026 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants