-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Markers: Save to CSV #3089
Markers: Save to CSV #3089
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3089 +/- ##
==========================================
- Coverage 88.76% 88.72% -0.04%
==========================================
Files 111 111
Lines 17246 17257 +11
==========================================
+ Hits 15308 15312 +4
- Misses 1938 1945 +7 ☔ View full report in Codecov by Sentry. |
@@ -4524,7 +4524,7 @@ def float_precision(column, item): | |||
# stored in astropy table as a float so we can also store nans, | |||
# but should display in the UI without any decimals | |||
return f"{item:.0f}" | |||
elif column in ('pixel', ): | |||
elif column in ('pixel', 'pixel_x', 'pixel_y'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mixin is quite generic so I left 'pixel'
in, just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this considered an API breaking change? That's probably ok for a 4.0 release, but maybe we need to be a little more "loud" or specific about it in the changelog? Otherwise, looks good to me!
Since we do not support reading this back in, I don't see this change being "breaking". |
But I wouldn't backport it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested this out and it looks good to me
@kecnry , are you still concerned about the change log? Any suggestion? |
[ci skip] [rtd skip] Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>
@kecnry , okay, I accepted your suggestion and cleaned it up. Thanks! Okay for my to squash merge when CI passes? |
Description
This pull request is to saving to CSV.
Change log entry
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, maintainershould 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.
trivial
label.