-
Notifications
You must be signed in to change notification settings - Fork 76
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
Re-enable exporting cubeviz spectrum viewer #2825
Re-enable exporting cubeviz spectrum viewer #2825
Conversation
c5ddd8c
to
68895ff
Compare
by removing any unicode characters and restoring after export
68895ff
to
efd9240
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2825 +/- ##
==========================================
+ Coverage 88.88% 88.91% +0.03%
==========================================
Files 111 111
Lines 16983 16985 +2
==========================================
+ Hits 15096 15103 +7
+ Misses 1887 1882 -5 ☔ View full report in Codecov by Sentry. |
Looking at the resulting PNG: "oh, it's that space that was causing problems!?" |
if len(getattr(mark, 'labels', [])): | ||
restore['labels'] = mark.labels[:] | ||
mark.labels = [lbl.strip() for lbl in mark.labels] | ||
restores.append(restore) |
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.
Could we also have restore_marks
and only populate the two lists if restore
isn't empty, to avoid looping over things we don't need to update in the zip
later?
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.
I'm not sure I follow - do you mean store a copy of the original mark? I'm trying to store/restore as little as possible to minimize memory impacts. I tried to avoid the append, but there can be some copying-referencing issues if we're not careful then.
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.
restores.append(restore) | |
if restore != {}: | |
restores.append(restore) | |
restore_marks.append(mark) |
Would restore_mark
have a copy of the object or a reference to the original in this case? I can never keep track of when python does which, but this would avoid looping over things that don't need to be restored. If memory is an issues it can stay as-is, I honestly don't think this is going to be impactful either way here.
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.
I suspect this would be more expensive than looping over the empty dictionary, but I'm not sure. We could also track indices, but I'm not sure its worth it either 🤷♂️
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.
As I said, I don't feel strongly about it, I'll just approve.
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.
Export works great! It feels the gap of unsupported Unicode in bqplot. Thanks!
Co-authored-by: Kyle Conroy <[email protected]>
Description
This pull request re-enables support for exporting the cubeviz spectrum viewer (to png or svg) by temporarily "cleaning" any unicode from the marks (which currently exist for the slice indicator).
The cleaning/restoring part of this PR could be reverted if support for unicode is fixed upstream (likely in bqplot).
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.