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

Better menu selection labels for spectral lines in line analysis plugin #2816

Merged
merged 7 commits into from
Apr 19, 2024

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Apr 17, 2024

Previously, the line menu under Redshift from Centroid showed the name and originally defined value for the lines, which could have been in different units. Now it shows name + rest value in current units + unit.

Before:
image

Now:
Screenshot 2024-04-17 at 2 58 02 PM

@rosteen rosteen added UI/UX😍 plugin Label for plugins common to multiple configurations labels Apr 17, 2024
@rosteen rosteen added this to the 3.10 milestone Apr 17, 2024
Comment on lines 252 to 253
self.line_items = msg.names_rest
self.line_items = [f"{msg.marks[i].name} {msg.marks[i].rest_value} {msg.marks[i].xunit}" for i in range(len(msg.marks))] # noqa
Copy link
Member

Choose a reason for hiding this comment

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

looks like this wasn't exposed API, so shouldn't cause backwards compatibility concerns 🤞

@rosteen rosteen force-pushed the units-in-redshift-from-centroid branch 2 times, most recently from 6786f9e to 10dbd0c Compare April 18, 2024 21:14
@rosteen
Copy link
Collaborator Author

rosteen commented Apr 18, 2024

I updated this to only change the displayed text in the menu, since changing the actual value returned as selected_line was going to cause a bunch of cascading changes that would end up being a huge headache.

Copy link
Contributor

@gibsongreen gibsongreen left a comment

Choose a reason for hiding this comment

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

Diff looks good to me and I really like the added clarity, nice work!

@rosteen rosteen force-pushed the units-in-redshift-from-centroid branch from a266ea5 to c28db42 Compare April 19, 2024 14:26
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.89%. Comparing base (a39a0fe) to head (c28db42).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2816   +/-   ##
=======================================
  Coverage   88.88%   88.89%           
=======================================
  Files         111      111           
  Lines       16983    16986    +3     
=======================================
+ Hits        15096    15099    +3     
  Misses       1887     1887           

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

@rosteen rosteen merged commit 446fa4f into spacetelescope:main Apr 19, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin Label for plugins common to multiple configurations Ready for final review specviz UI/UX😍
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants