Display units: line lists

[kecnry]

Description

This pull request updates the line lists plugin so that existing lines are unit-aware. This does not yet update the display of lines in the plugin itself to show in the display units - that work will likely be deferred for the refactor of the line list plugin as both of those efforts will require changing the way the information is stored internally.

Screen.Recording.2023-04-12.at.3.57.14.PM.mov

Changelog will point to #2127 so is ignored from CI here.

Closes #2028

Change log entry

• Is a change log needed? If yes, is it added to 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, maintainer
  should 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.

• 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 milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[javerbukh]

I tried adding custom lines after changing the units and also adding custom lines in different units and then switching to those units. Everything worked as expected. I did notice that the table at the bottom of the plugin showing rest and observed units could potentially be updated if it made scientific sense. Should the observed value be in the original units of the line or should it update with the spectral unit? Since I don't know if that is a valid question and everything else looks good I'll go ahead and approve!

[kecnry]

I did notice that the table at the bottom of the plugin showing rest and observed units could potentially be updated if it made scientific sense. Should the observed value be in the original units of the line or should it update with the spectral unit?

I think we'll eventually want this (at least as the default), but I think it makes sense to defer until the refactor (which will make use of a database of all lines, etc).

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (dev-display-units@819ed17). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@                 Coverage Diff                  @@
##             dev-display-units    #2148   +/-   ##
====================================================
  Coverage                     ?   91.96%           
====================================================
  Files                        ?      146           
  Lines                        ?    15960           
  Branches                     ?        0           
====================================================
  Hits                         ?    14678           
  Misses                       ?     1282           
  Partials                     ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[rosteen]

Thought I had approved this, sorry. I tested and saw that everything worked on Friday, let me do one more pass of the actual code before I give it a check mark.

[rosteen]

Wait, this doesn't inherit anything...does the base python Class init handle setting self.[whatever] for kwargs if you call super().__init__ like this?

[kecnry]

turns out its required in order for the comm to be set correctly when other classes inherit from this and Lines

[rosteen]

I removed the other reviewer tags since I'm 2, I think that's at least in the direction of what we agreed on for avoiding overloading reviews 😃

[rosteen]

I did one more quick test post-changes and it still works, thanks!