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

Only add 2nd Best Fit option if it actually exists #100

Merged
merged 6 commits into from
May 3, 2024

Conversation

weaverba137
Copy link
Member

This PR fixes #98.

When Prospect is used with SDSS data or data derived from SPARCL, a 2nd best fit model is not available. However, ViewerWidgets.add_model_select() was assuming that it was always available based on with_full_2ndfit which defaults to True.

This resulted in the JavaScript side of prospect attempting to obtain data from an object that is null (a Python None converted to JavaScript).

With this change, the argument with_full_2ndfit is unused, can it be eliminated from that method entirely?

@weaverba137 weaverba137 requested a review from armengau May 2, 2024 20:38
@weaverba137 weaverba137 self-assigned this May 2, 2024
@coveralls
Copy link

coveralls commented May 2, 2024

Coverage Status

coverage: 2.008% (-0.002%) from 2.01%
when pulling 0c76f6e on missing-2nd-model
into aa69275 on main.

@armengau
Copy link
Collaborator

armengau commented May 3, 2024

Agree with the change (and removed arg with_full_2ndfit).

Now I understand what happened: indeed you had redrock_cat=None, an option previously incompatible with with_full_2ndfit=True (which is default...). I never stubbled on that because the script prospect_pages.py explicitely prevented this, with some hard-coded settings in the script.

I took this opportunity to remove this hard-coding.

I tested the pages quite extensively, on my side it's ok now.

@armengau
Copy link
Collaborator

armengau commented May 3, 2024

.. But please check one last time on your notebook!

@weaverba137
Copy link
Member Author

Testing now. If all goes well, I'll merge shortly.

@weaverba137
Copy link
Member Author

Tests look good, so I will merge momentarily.

@weaverba137 weaverba137 merged commit 4176d04 into main May 3, 2024
10 checks passed
@weaverba137 weaverba137 deleted the missing-2nd-model branch May 3, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plot synchronization issues: "ghost" spectra and arrow/slider sync.
3 participants