BUG: Fix parsing of MANGA cube (try 2)

[pllim]

Description

This pull request is to fix loading MANGA cube in Cubeviz. Since Brian C said it works on Jdaviz 3.10 series, probably no need to backport.

This supersedes #3115 and takes advantage of the loaders that already exist in specutils when we can, like Brian Cherinka suggested. Also convert uncert type to stddev when we can, like Brett Morris suggested.

I tried to get rid of _parse_hdulist altogether but too many tests failed, so I kept it as a fallback.

Also fixed a Marker display bug where wavelength shows as 0.000 m, which is very uninformative.

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. Bugfix milestone also needs an accompanying backport label.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.89%. Comparing base (5a020cc) to head (b80fb8a).
Report is 144 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3119      +/-   ##
==========================================
+ Coverage   88.88%   88.89%   +0.01%     
==========================================
  Files         112      112              
  Lines       17391    17396       +5     
==========================================
+ Hits        15458    15465       +7     
+ Misses       1933     1931       -2     

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

[pllim]

@havok2063 , would this work for your use case? Can you please try it out? Thanks!

[havok2063]

Tested this on a few manga cubes and it works for me well enough. This fix and approach seems reasonable to me.

[kecnry]

can we do anything more specific than general exception that won't result in catching generic typos, etc?

[pllim]

@rosteen , any idea? I don't think there is a way to know if any arbitrary input is going to succeed/fail the specutils I/O registry until you actually do it.

[kecnry]

but is there even a specific extension exception we can catch instead of the catch-all?

[pllim]

If you mean exception, I purposely make it general here to be backward compatible. If specutils cannot parse for whatever reason, it goes back to the old way (until we can get rid of it because I think we should).

[pllim]

"But is there a cleaner solution?"

Yes, there is. We can write a custom parser for each format that specutils cannot read. But I don't have that list or time.

[kecnry]

My concern is just that some random bug or typo could result in the except always being triggered and we have no way of knowing that or noticing it in the tests. Maybe for now just a snackbar if the except is entered and then could check for that snackbar message in test coverage could do?

[pllim]

I do not see how that is worse than what we already have. Currently, it goes through our own HDU parser anyway. With this change, it would still do that and then still crash like it would currently if both specutils and HDU parser cannot handle it. The only difference introduced by this PR is that it tries specutils first. A snackbar message before fallback would just be confusing if it ends up succeeding later.

[kecnry]

agreed from the user-perspective, I'm more concerned about testing and ending up with stale code without noticing (or just not clear which path a file took). But as long as there is a plan to consolidate these later, I guess this is fine for now. Thanks!

[pllim]

I think @rosteen is going to end up refactoring this for his specutils 2.0 work?

[kecnry]

same as above

[pllim]

Replied at #3119 (comment)

[pllim]

Rebased to get rid of merge conflicts.

[pllim]

Thanks for the reviews!