[BUG] Specviz: Line list filter toggle doesn't seem to handle differing units

[rosteen]

Describe the bug The "filter to visible" toggle in the Line List plugin filters everything out, even when some lines are visible. I suspect it's because the viewer x-axis is in meters while the rest wavelengths are in Angstrom, but I'm not sure.

To Reproduce Steps to reproduce the behavior:

1. Run the CubevizExample notebook
2. Load the Common Stellar line list, plot some lines that are visible on the viewer
3. Click the "Filter to visible range" toggle, see that everything gets filtered out.

Expected behavior The lines visible in the viewer would remain after filtering in the plugin.

Screenshots

🐱

[pllim]

I think the bug is here:

https://github.com/spacetelescope/jdaviz/blob/af4c97466c43618b1bb55ea10a0b648f34c58746/jdaviz/configs/default/plugins/line_lists/line_lists.vue#L363

.obs is in Angstrom but .spectrum_viewer_min/max is in the converted unit. We could force the latter to be in Angstrom here:

https://github.com/spacetelescope/jdaviz/blob/af4c97466c43618b1bb55ea10a0b648f34c58746/jdaviz/configs/default/plugins/line_lists/line_lists.py#L379-L380

But that will break if someone provides a line list in another wavelength unit. Because the unit is separate from the value in the code, it seems pretty brittle. If we could refactor this, we should use astropy.units.Quantity as much as possible so the unit is always attached to the value.

I am not sure how to fix this quickly in a robust way. Maybe @kecnry has a better idea since he worked on this a lot lately.

[kecnry]

My suggestion would be to expose these traitlets (both axes limits and line location) to Vue in fixed units (say Angstroms), separately from the rest/observed wavelength in user-provided units. That way the filter logic in JS does not need to worry about units at all. That does mean an extra field per-line, but I don't think that should cause any issues.

Hopefully if the axes limits are sorted, then we won't need to worry about wavelength vs frequency units either.

[duytnguyendtn]

Ahh this is much cleaner than my solution I was working on of having getters that convert the units. ~I can do a quick PR for this one~

[duytnguyendtn]

Hold up, but we would need to convert line.obs into this base unit (Angstroms as per @kecnry's example), right? We were having issues trying to get data back from the python side to the JS side so I don't know how to get that back

[pllim]

Yeah, if you convert everything to a common unit on parse, then it will simplify some code. But then if you want to save those values back out in original unit, you wanna watch out for roundtripping issues -- if that is even a use case you support.