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

Fix incorrect parsing of the ATCA FITS header #1376

Merged
merged 3 commits into from
Jun 3, 2024

Conversation

markccchiang
Copy link
Contributor

@markccchiang markccchiang commented May 10, 2024

Description

  • What is implemented or fixed? Mention the linked issue(s), if any.
    Closes incorrect trimmed velocity header of ATCA FITS images #1375.

  • How does this PR solve the issue? Give a brief summary.
    The value of SPECSYS from the ATCA FITS header is already provided. So we don't need to parse it from the suffix of VELO-XXX (the string after '-'), which would also result in the failure rendering of the coordinate system.

  • Are there any companion PRs (frontend, protobuf)?
    No.

  • Is there anything else that testers should know (e.g. exactly how to reproduce the issue)?
    The coordinate system for ATCA FITS images can be successfully rendered on the frontend.

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

@pford
Copy link
Collaborator

pford commented May 15, 2024

Should the original ctype value VELO-XXX also be unchanged? The issue requested the original header. I think I wrote this code, not sure why I changed the value but maybe a casacore issue at the time.

Also if the SPECSYS header is read before the CTYPE header, then the specsys string will be replaced with the CTYPE value, which is not what we want.

Starting at line 319 in this branch FileExtInfoLoader.cc:

        } else if (value.startsWith("VELO-") && specsys.empty()) {   <== only if not set?
            specsys = value.after('-');
            value = "VELO";     <=== remove this line?
        }

@markccchiang
Copy link
Contributor Author

markccchiang commented May 16, 2024

I noticed that if I do not remove the string after VELO (from the original VELO-XXX), the x-label of the spectral profile plot can not be shown normally. Like the following snapshot:

截圖 2024-05-16 上午10 11 05

with "CTYPE3 = VELO" the x-label is normally shown:

截圖 2024-05-16 上午10 12 33

Not sure if this is the AST problem, or just the frontend code issue(?) @kswang1029 @TienHao do you have comments on it?

@TienHao
Copy link

TienHao commented May 16, 2024

Not sure if this is the AST problem, or just the frontend code issue(?) @kswang1029 @TienHao do you have comments on it?

Yeah, the frontend code needs to be modified for VELO-XXX. It does not give spectral type name for VELO-XXX.

@kswang1029, do VELO-XXX have the same spectral type name and unit just like VELO? or they should have other names?

@kswang1029
Copy link
Contributor

Not sure if this is the AST problem, or just the frontend code issue(?) @kswang1029 @TienHao do you have comments on it?

Yeah, the frontend code needs to be modified for VELO-XXX. It does not give spectral type name for VELO-XXX.

@kswang1029, do VELO-XXX have the same spectral type name and unit just like VELO? or they should have other names?

Yes just like VELO. The VELO-XXX is a (old?) way to encode the spectral reference frame. We still refer CUNIT3 for the unit.

@pford
Copy link
Collaborator

pford commented May 16, 2024

I approved the code review, and will leave it up to the others whether to keep the change to "VELO" or show the actual CTYPE header value. It sounds like the suffix is outdated, does not add any information since SPECSYS is defined, and requires a workaround for the spectral profile.

@kswang1029 kswang1029 added the requiring frontend this backend issue requires work in both frontend and backend label May 20, 2024
@TienHao
Copy link

TienHao commented May 22, 2024

Since the frontend is working with CTYPE3 = VELO in the current PR(no VELO-XXX is sent through headers), I think there is no need to change in the frontend for the spectral type name now.

@confluence confluence self-assigned this May 22, 2024
@confluence confluence added the awaiting testing For pull requests that require testing label May 22, 2024
@YuHsuan-Hwang
Copy link
Contributor

Does this PR still need frontend changes?

@kswang1029
Copy link
Contributor

Does this PR still need frontend changes?

I will check and report back.

@kswang1029
Copy link
Contributor

Does this PR still need frontend changes?

No, we do not need a corresponding frontend change. 👍

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.

issue fixed. no regression from e2e tests. corresponding frontend change is not needed.

@kswang1029
Copy link
Contributor

@confluence ready to merge after fixing the changelog conflict.

@kswang1029 kswang1029 added awaiting merge For pull requests ready for merge or pending frontend/protobuf changes and removed awaiting testing For pull requests that require testing requiring frontend this backend issue requires work in both frontend and backend labels Jun 3, 2024
Copy link

github-actions bot commented Jun 3, 2024

Code Coverage

Package Line Rate Health
src.Cache 72%
src.DataStream 44%
src.FileList 67%
src.Frame 36%
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 40%
Summary 46% (8618 / 18802)

@confluence confluence merged commit e20a627 into dev Jun 3, 2024
14 checks passed
@confluence confluence deleted the mark/1375_fix_velocity_header_for_ATCA_FITS branch June 3, 2024 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge For pull requests ready for merge or pending frontend/protobuf changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incorrect trimmed velocity header of ATCA FITS images
6 participants