BUG: Allow data_label as pos arg

[pllim]

Description

This pull request is to allow common user mistake passing in data_label as positional argument. This approach is kinda hacky but is less disruptive than throwing error if data_label is not a keyword argument.

Fixes #1617

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 change log needed? If yes, is it added to CHANGES.rst?
• Is a milestone set?
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[codecov]

Codecov Report

Base: 86.35% // Head: 86.36% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (9dd46ea) compared to base (1cb9d95).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1644   +/-   ##
=======================================
  Coverage   86.35%   86.36%           
=======================================
  Files          95       95           
  Lines        9627     9634    +7     
=======================================
+ Hits         8313     8320    +7     
  Misses       1314     1314           

Impacted Files	Coverage Δ
jdaviz/app.py	91.80% <ø> (ø)
jdaviz/configs/specviz/helper.py	56.62% <ø> (ø)
jdaviz/configs/cubeviz/helper.py	96.42% <100.00%> (+0.13%)	⬆️
jdaviz/configs/imviz/helper.py	96.66% <100.00%> (+0.04%)	⬆️
jdaviz/configs/mosviz/helper.py	86.77% <100.00%> (ø)
jdaviz/core/helpers.py	66.66% <100.00%> (+0.28%)	⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kecnry]

I'm worried this is a slippery slope... although in this case it might be worth the convenience. Is switching the order of the positional arguments too big of an API break? If we do stick with this, should we at least raise a warning?

[pllim]

raise a warning?

Hmm. Maybe @rosteen can chime in here since he encountered this use case.

[rosteen]

Personally I think we should just switch the order of the keyword arguments so data_label comes first, since all the helper load_data methods use the parser_reference keyword explicitly and don't depend on the position of that keyword. Supporting the user ease of use case is more important than our internal use anyway (I don't think any user will ever use parser_reference).

[pllim]

Currently data_label is hidden in the **kwargs. I'll have to ponder.

[rosteen]

Right, I vote for pulling it out of **kwargs and making it explicit so it can be used positionally.

[pllim]

Re: #1644 (comment)

This is made complicated that mosviz parser accepts data_labels and not data_label. Making it explicit is going to overcomplicate things. Any other ideas?

[rosteen]

I think that the Mosviz helper methods can be left as-is and not be affected by this. For example, load_1d_spectra already accepts data_labels explicitly as the second positional argument, and then passes it to app.load_data as a keyword. So it doesn't really matter if we put data_label=None in front of parser_reference elsewhere, the Mosviz parsers can just not care about data_label and get data_labels same as they do currently.

[pllim]

But what if you pass in data_label='something' to Mosviz (muscle memory from using other viz, etc) and then wonder why things don't work?

[rosteen]

Well, I think I'd argue that that's already possible and fixing it is out of scope for this PR 😆 . Plus, if data_label could be positional they might not be in the habit of naming it explicitly!

Could always catch data_label being defined in the Mosviz helper methods and redirecting the value to data_labels I guess if we're really worried about it.

[pllim]

I'll have to think about this. If we make it explicit, it will also break existing API passing in parser_reference as positional argument.

[pllim]

Okay... I completely changed the implementation. Please re-review. Thanks!

[rosteen]

Looks good to me now, thanks!

[kecnry]

I'd personally like to also see the orders in mosviz and specviz2d changed to parallel this more closely, but realize that would definitely be considered an API-breaking change and require a lot more thought... so let's leave those out for now.

Specviz already supports data_label as the second positional argument in its load_spectrum. We may also want to consider renaming that (with deprecation) to load_data to be more consistent, but that can also be handled later.

Was parser_reference ever passed anywhere in docs/examples notebooks that would justify the need for deprecation or a version bump here? Otherwise, I don't see any issues with this as-is in a minor release. Definitely will avoid some possible confusion when a user first start using jdaviz!

[pllim]

No, I think adding parser_reference to Imviz.load_data was a mistake on my part. And nowhere in the doc we ask people to use it. So I think we can change this for Imviz without any deprecation. Thanks for the reviews!

Deprecating Specviz.load_spectrum might be controversial because it is so widely used. If you feel strongly we should do it, please open a new ticket.

[kecnry]

Deprecating Specviz.load_spectrum might be controversial because it is so widely used. If you feel strongly we should do it, please open a new ticket.

I agree, we'd definitely need a long (perhaps infinitely long) deprecation period. At least having the alias for load_data and switching example notebooks to use that might be a good idea. But also probably not high on the priority list 🤷