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

TST: Re-enable test_moment_maps #1024

Merged
merged 2 commits into from
Jan 4, 2022

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Jan 3, 2022

Description

This pull request is to re-enable and fix a failing test.

Fixes #948

Follow-up of #547

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 change log needed? If yes, is it added to CHANGES.rst?
  • Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@pllim pllim added the no-changelog-entry-needed changelog bot directive label Jan 3, 2022
@pllim pllim added this to the 2.3 milestone Jan 3, 2022
@pllim

This comment has been minimized.

@pllim pllim added the testing label Jan 3, 2022
@pllim
Copy link
Contributor Author

pllim commented Jan 3, 2022

I also noticed a lot of this:

WARNING specutils.spectra.spectrum1d:spectrum1d.py:205 Input WCS indicates that the spectral axis is not last. Reshaping arrays to put spectral axis last.

I suspect there are some unnecessary data translation going on in the background but fixing that is out of scope here.

@codecov
Copy link

codecov bot commented Jan 3, 2022

Codecov Report

Merging #1024 (dc92c56) into main (9fb18f6) will increase coverage by 0.40%.
The diff coverage is n/a.

❗ Current head dc92c56 differs from pull request most recent head a32ddca. Consider uploading reports for the commit a32ddca to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1024      +/-   ##
==========================================
+ Coverage   72.69%   73.09%   +0.40%     
==========================================
  Files          75       75              
  Lines        5669     5669              
==========================================
+ Hits         4121     4144      +23     
+ Misses       1548     1525      -23     
Impacted Files Coverage Δ
...configs/cubeviz/plugins/moment_maps/moment_maps.py 78.15% <0.00%> (+19.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fb18f6...a32ddca. Read the comment docs.

@pllim pllim marked this pull request as ready for review January 3, 2022 23:06
@pllim

This comment has been minimized.

@pllim pllim force-pushed the a-moment-like-this branch from dc92c56 to a32ddca Compare January 4, 2022 16:17
result = dc[1].get_object(cls=CCDData)
assert result.shape == (4, 2)

# FIXME: Need spatial WCS, see https://github.com/spacetelescope/jdaviz/issues/1025
Copy link
Contributor Author

Choose a reason for hiding this comment

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

xref #1025

@rosteen rosteen merged commit bc775f0 into spacetelescope:main Jan 4, 2022
@pllim pllim deleted the a-moment-like-this branch January 4, 2022 16:49
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.

Moment map tests fail with spectrum1d cubes
3 participants