Skip to content

View spectral info in 2D images - #1134

Closed
debora-pe wants to merge 9 commits into
ejeschke:mainfrom
debora-pe:spectral_view
Closed

View spectral info in 2D images#1134
debora-pe wants to merge 9 commits into
ejeschke:mainfrom
debora-pe:spectral_view

Conversation

@debora-pe

Copy link
Copy Markdown
Contributor

Description

This pull request is to add the possibility to view rectified 2D spectral images that have wavelength information recorded in the header. With this, the wavelength value in each pixels is displayed.

@ejeschke
ejeschke self-requested a review November 22, 2025 11:22
@ejeschke ejeschke self-assigned this Nov 22, 2025
@ejeschke ejeschke added this to the v5.5.0 milestone Nov 22, 2025

@ejeschke ejeschke left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hi @debora-pe , thanks for this PR! I have small request to make the extraction of the wavelength more efficient by caching the wavelength array after you build it. Could you try my suggestion in the review?

In the future we might be able to improve it further by building the wavelength array when the image is loaded if we detect the correct FITS headers.

Comment thread ginga/AstroImage.py
waveimg = np.tile(wave_array[:, np.newaxis], _wd)
else:
raise ValueError(f"Unsupported spectral axis: {spec_axis}")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@debora-pe , thanks for this very useful PR!

We should look at either A) building the wavelength array when the AstroImage is loaded or a WCS is set, or B) building and caching it in the AstroImage in this function. Otherwise it is constantly being built and rebuilt as the user sweeps the cursor across the image. The former would be cleaner, but the latter is not too bad. There is a metadata dictionary in every AstroImage which would be the logical place to store this.

Example here, just after you have created the wavelength array:

self.set(wavelen_array=waveimg)

Then, see the check above, to avoid having to rebuild the array. Let me know if you think this would work ok (maybe give it a try and update this PR)?

Comment thread ginga/AstroImage.py
return self.wcs.radectopix(ra_deg, dec_deg, coords=coords,
naxispath=self.revnaxis)

def pixtospec(self, x, y):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

(See comment below)

        # check for cached wavelength array
        waveimg = self.get('wavelen_array', None)
        if waveimg is None:
               # build wavelength array

@debora-pe

Copy link
Copy Markdown
Contributor Author

Hi @debora-pe , thanks for this PR! I have small request to make the extraction of the wavelength more efficient by caching the wavelength array after you build it. Could you try my suggestion in the review?

In the future we might be able to improve it further by building the wavelength array when the image is loaded if we detect the correct FITS headers.

Thanks @ejeschke for your comments. I follow your suggestion and it seems working well. Thanks!

@ejeschke

Copy link
Copy Markdown
Owner

@debora-pe, I had to make a white space correction to get it to pass flake8 tests. Merged in #1135 with you as co-author.

@ejeschke ejeschke closed this Nov 24, 2025
@pllim

pllim commented Nov 24, 2025

Copy link
Copy Markdown
Collaborator

@ejeschke for future reference, unless the PR author specifically uncheck a box, you could have pushed your change straight to this PR branch instead of making a new PR off this branch. FYI.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@ejeschke

Copy link
Copy Markdown
Owner

@ejeschke for future reference, unless the PR author specifically uncheck a box, you could have pushed your change straight to this PR branch instead of making a new PR off this branch. FYI.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

I tried, but it wouldn't let me. I think the owner of the repo needs to allow that.

@ejeschke

Copy link
Copy Markdown
Owner

It's possible I didn't try the right method. There was also something about being able to fix small typos and so forth directly in the PR by clicking the Edit option on the file being showed in the "Files Changed", but that was greyed out for me.

@pllim

pllim commented Nov 24, 2025

Copy link
Copy Markdown
Collaborator

Hmm. Well, too late for this PR now, but if it comes up again, I'll be happy to help debug. Thanks!

@ejeschke

Copy link
Copy Markdown
Owner

Credit was properly given, and @debora-pe is now a contributor

@pllim

pllim commented Nov 24, 2025

Copy link
Copy Markdown
Collaborator

It wasn't meant as accusation or anything. Sorry if I came across otherwise. Thanks, all!

@ejeschke

Copy link
Copy Markdown
Owner

Sorry if I came across otherwise.

Not at all, no worries!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants