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

Cubeviz to use wcs1d-fits from specutils to write out fitted cube #2094

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Mar 17, 2023

Description

This pull request is to remove the unreleased jdaviz-cube writer we added in #2012 and use the improved specutils wcs1d-fits writer that Derek modifed in astropy/specutils#1009 .

Close #2044

Blocked by

Change log entry

  • Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
    list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
    should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@pllim pllim added the skip-changelog-checks changelog bot directive label Mar 17, 2023
@pllim pllim added this to the 3.4 milestone Mar 17, 2023
@github-actions github-actions bot added cubeviz documentation Explanation of code and concepts labels Mar 17, 2023
@rosteen rosteen modified the milestones: 3.4, 3.5 Mar 22, 2023
@dhomeier
Copy link
Collaborator

@pllim astropy/specutils#1051 has now passed all tests, so you might try it for the bool mask here.

@pllim

This comment was marked as resolved.

@pllim pllim force-pushed the cubeviz-use-wcs1d-fits branch 2 times, most recently from 619cd58 to 1fba94e Compare March 30, 2023 14:25
@pllim pllim force-pushed the cubeviz-use-wcs1d-fits branch from 1fba94e to e16a5d5 Compare April 7, 2023 20:26
@pllim pllim added api API change and removed skip-changelog-checks changelog bot directive labels May 10, 2023
@pllim pllim force-pushed the cubeviz-use-wcs1d-fits branch from e16a5d5 to d75de7a Compare May 10, 2023 17:29
@pllim

This comment was marked as resolved.

@dhomeier

This comment was marked as resolved.

@haticekaratay haticekaratay modified the milestones: 3.5, 3.6 May 25, 2023
@javerbukh javerbukh modified the milestones: 3.6, 3.7 Jul 28, 2023
@pllim pllim modified the milestones: 3.7, 3.8 Sep 21, 2023
@rosteen rosteen modified the milestones: 3.8, 3.9 Nov 29, 2023
@gibsongreen gibsongreen modified the milestones: 3.9, 3.10 Apr 5, 2024
@bmorris3 bmorris3 modified the milestones: 3.10, 3.11 May 3, 2024
@pllim pllim force-pushed the cubeviz-use-wcs1d-fits branch from d75de7a to 3b00e22 Compare July 29, 2024 18:42
@pllim pllim modified the milestones: 3.11, 4.0 Jul 29, 2024
Copy link

codecov bot commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.88%. Comparing base (a126296) to head (9d46233).
Report is 148 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2094      +/-   ##
==========================================
- Coverage   88.93%   88.88%   -0.06%     
==========================================
  Files         112      112              
  Lines       17411    17391      -20     
==========================================
- Hits        15485    15458      -27     
- Misses       1926     1933       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pllim pllim marked this pull request as ready for review July 29, 2024 19:13
@pllim
Copy link
Contributor Author

pllim commented Jul 29, 2024

I think at long last, this PR is finally ready because upstream fix has been released since.

@pllim
Copy link
Contributor Author

pllim commented Jul 29, 2024

Not sure why RTD status report is stuck (PR too old? lol) but the build was successful: https://readthedocs.org/projects/jdaviz/builds/25146976/

@rosteen
Copy link
Collaborator

rosteen commented Jul 30, 2024

Hm, this works for writing out a model fit cube, but interestingly, it fails if you try to write out the original data cube that was loaded into Cubeviz (at least that's what I'm seeing with the example notebook). That also fails to write on main with format="jdaviz-cube" though, and based on the title of this PR is maybe out of scope.

@pllim
Copy link
Contributor Author

pllim commented Jul 30, 2024

it fails if you try to write out the original data cube that was loaded into Cubeviz

Pretty sure that was a separate ticket. I don't think we support complete I/O roundtripping anywhere yet.

@rosteen
Copy link
Collaborator

rosteen commented Jul 30, 2024

it fails if you try to write out the original data cube that was loaded into Cubeviz

Pretty sure that was a separate ticket. I don't think we support complete I/O roundtripping anywhere yet.

In that case I'll go ahead and approve this.

@pllim pllim force-pushed the cubeviz-use-wcs1d-fits branch from 3b00e22 to 9d46233 Compare July 30, 2024 19:38
@pllim
Copy link
Contributor Author

pllim commented Jul 30, 2024

Rebased to see if RTD would report back this time. Thanks for the review!

@pllim pllim enabled auto-merge July 30, 2024 19:40
@pllim pllim merged commit 5a020cc into spacetelescope:main Jul 30, 2024
16 checks passed
@pllim pllim deleted the cubeviz-use-wcs1d-fits branch July 30, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API change cubeviz documentation Explanation of code and concepts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jdaviz-cube writer should also write out uncertainty
7 participants