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

Update __str__ and __repr__ for Spectrum1D #1123

Merged
merged 15 commits into from
Feb 13, 2024

Conversation

rosteen
Copy link
Contributor

@rosteen rosteen commented Feb 6, 2024

Fixes #1073, as well as improving the readability/concision of the __repr__ format (especially for things like large cubes). Still tweaking this but it's close, tests will need updating as well.

@rosteen rosteen added this to the v1.x milestone Feb 7, 2024
@rosteen rosteen added bug docs data objects Core data objects like Spectrum1D or SpectralCollection labels Feb 7, 2024
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (425eab3) 71.07% compared to head (4019868) 70.93%.

❗ Current head 4019868 differs from pull request most recent head 1e00460. Consider uploading reports for the commit 1e00460 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1123      +/-   ##
==========================================
- Coverage   71.07%   70.93%   -0.14%     
==========================================
  Files          61       61              
  Lines        4249     4215      -34     
==========================================
- Hits         3020     2990      -30     
+ Misses       1229     1225       -4     

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

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

I like the simplification but... have you tested this with numpy 2.0 (from their nightly wheel)?

@pllim
Copy link
Member

pllim commented Feb 8, 2024

Looks like your devdeps job pulls in numpy 2.0.dev but I also see warnings, so please inspect the log carefully.

https://github.com/astropy/specutils/actions/runs/7832040522/job/21369775774?pr=1123

@rosteen rosteen marked this pull request as ready for review February 12, 2024 14:36
@rosteen
Copy link
Contributor Author

rosteen commented Feb 12, 2024

Looks like your devdeps job pulls in numpy 2.0.dev but I also see warnings, so please inspect the log carefully.

https://github.com/astropy/specutils/actions/runs/7832040522/job/21369775774?pr=1123

As far as I could tell the only warning was from an upstream package, nothing to do with this. This PR actually fixes some numpy-related errors, although I think that's due to a pre-2.0 update. I got a thumbs-up for this offline so I'll probably merge after tests pass rather than wait for more input (unless you want to give feedback on the actual changes, Pey Lian).

@pllim
Copy link
Member

pllim commented Feb 13, 2024

I think the actual diff looks reasonable. I think the new str and repr are nicer. Thanks!

@rosteen
Copy link
Contributor Author

rosteen commented Feb 13, 2024

Test failure was a timeout, merging.

@rosteen rosteen merged commit c646007 into astropy:main Feb 13, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug data objects Core data objects like Spectrum1D or SpectralCollection docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spectrum1D.__str__ formatting error
2 participants