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

Aggregate information about image axes and dimensions into structs #1392

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

confluence
Copy link
Collaborator

Description

This is a refactoring PR which aggregates information about image axes and dimensions into two structs. This simplifies passing the information around and storing it as properties: we can use the structs instead of inconsistent subsets of individual axes and dimensions.

Major changes:

  • FileLoader::::FindCoordinateAxes signature has changed; it now only sets _axes and _dims on the loader and does not also modify return parameters (but it's still a bool and may set the message parameter).
  • Individual axes properties in FileLoader and Frame replaced with structs. Individual getters have been kept; getters for the structs have been added.
  • Calculation of dims from axes and shape moved to dims struct constructor to enable cleaner reuse from multiple places where axes can be calculated.
  • Frame, HDF5 loader, polarization calculator, Stokes file connector, extended file info loader updated to use axes and/or dims structs.

There are two non-refactoring changes:

  1. While I was making this change I noticed that previously the spectral axis of a compressed FITS file was being set to the depth axis, and have fixed this. At the moment compressed FITS files with rotated axes can't be viewed at all for unrelated reasons; I will see how easy that would be to fix, and possibly factor these changes out into a separate PR.
  2. FileLoader::::FindCoordinateAxes now explicitly bails out with an error message if there are fewer than 2 render axes (e.g. in a weird case like a 2D image with one Stokes axis). Previously I believe that this would have crashed. Maybe we should give images like this a second render axis with size 1 (like we handle images with no depth or Stokes), but that would be a much more complicated change (and it may not be something we need).

Possible outstanding issue

  • There are hardcoded axes in Frame's image export functions. Should these be updated to support rotated axes?

Checklist

  • changelog updated / no changelog update needed
  • e2e test passing / corresponding fix added / new e2e test created
  • ICD test passing / corresponding fix added / new ICD test created
  • protobuf updated to the latest dev commit / no protobuf update needed
  • protobuf version bumped / protobuf version not bumped
  • added reviewers and assignee
  • GitHub Project estimate added

@confluence confluence self-assigned this Nov 8, 2024
Copy link
Collaborator

@pford pford left a comment

Choose a reason for hiding this comment

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

Remove spectral and stokes axis from CompressedFits

src/FileList/FileExtInfoLoader.cc Outdated Show resolved Hide resolved
@pford
Copy link
Collaborator

pford commented Nov 9, 2024

I could not find anything that was missed.
I can fix the Save Image bug for issue #1178 (additional support for rotated cubes).

Copy link

github-actions bot commented Nov 9, 2024

Code Coverage

Package Line Rate Health
src.Cache 72%
src.DataStream 44%
src.FileList 67%
src.Frame 35%
src.HttpServer 42%
src.ImageData 28%
src.ImageFitter 83%
src.ImageGenerators 43%
src.ImageStats 75%
src.Logger 37%
src.Main 52%
src.Region 69%
src.Session 4%
src.Table 52%
src.ThreadingManager 67%
src.Timer 85%
src.Util 38%
Summary 46% (8638 / 18950)

@confluence confluence marked this pull request as ready for review November 15, 2024 17:11
@confluence confluence added the awaiting testing For pull requests that require testing label Nov 15, 2024
Copy link
Contributor

@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.

Loading individual Stokes images as a hypercube does not work. This is picked up by e2e. Requesting @pford a re-review once addressed.
selenium-screenshot-5

The test images in the screenshot all have Shape = [256, 256, 480, 1] (RA, DEC, FREQ, STOKES).

Other tests images with Shape = [256, 256, 480] (RA, DEC, FREQ) or Shape = [6280, 3140] (GLON, GLAT) work however.

@kswang1029 kswang1029 requested a review from pford December 10, 2024 05:44
@kswang1029 kswang1029 added awaiting code changes For pull requests that require code changes and removed awaiting testing For pull requests that require testing labels Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting code changes For pull requests that require code changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants