astro-datalab / notebooks-latest

Default set of Data Lab notebooks, by DL team and contributed by users
BSD 3-Clause "New" or "Revised" License
57 stars 48 forks source link

Fix fully-masked SDSS spectrum #213

Closed weaverba137 closed 8 months ago

weaverba137 commented 9 months ago

This PR adds a test for fully-masked spectra, which cause problems for current versions of Prospect. The test is added to DESI and BOSS spectra as well, for completeness.

This notebook also demonstrates how to hide only a particular class of warning that we don't care about.

Please test, then we'll update the HTML file, and add the cell outputs back into the notebook.

weaverba137 commented 8 months ago

@stephjuneau, I've implemented your suggestions, although the HTML version remains to be generated. I've applied the fully-masked check to all of DESI, SDSS, BOSS. Please test.

weaverba137 commented 8 months ago

I see that two notations are used for defining the subset of spectra/models to display. They seem equivalent as both "work" but we could consider homogenizing for stylistic consistency. For example: [~desi_fully_masked, :] versus [~desi_fully_masked] both seem to work in the same way when applied to, e.g., flux (unless this is only true when selecting everything; I haven't experimented extensively)

Those objects have different dimensions, so the different notations are actually completely necessary.

The Prospect window shows "other model" in dashed line for DESI (perhaps expected?) but also for SDSS

I will take a look at that.

Just noticed this but it may have been there before and be a Prospect issue but navigating to the ...

I believe that is a Prospect issue outside the scope of this notebook.

stephjuneau commented 8 months ago

I see that two notations are used for defining the subset of spectra/models to display. They seem equivalent as both "work" but we could consider homogenizing for stylistic consistency. For example: [~desi_fully_masked, :] versus [~desi_fully_masked] both seem to work in the same way when applied to, e.g., flux (unless this is only true when selecting everything; I haven't experimented extensively)

Those objects have different dimensions, so the different notations are actually completely necessary.

OK but the case that first caught my eye was: sdss_model = (sdss_spectra.records[0].wavelength, model_flux[~sdss_fully_masked, :]) having the same notation for boss but for desi: desi_model = (desi_spectra.records[0].wavelength, model_flux[~desi_fully_masked]) without the ,:. Is this expected?

The Prospect window shows "other model" in dashed line for DESI (perhaps expected?) but also for SDSS

I will take a look at that.

OK thanks!

Just noticed this but it may have been there before and be a Prospect issue but navigating to the ...

I believe that is a Prospect issue outside the scope of this notebook.

Sure, could you create an issue for tracking this separately somewhere?

weaverba137 commented 8 months ago

The Prospect window shows "other model" in dashed line for DESI (perhaps expected?) but also for SDSS desi_model = (desi_spectra.records[0].wavelength, model_flux[~desi_fully_masked])

Thanx, that was a typo. I've fixed that one.

The Prospect window shows "other model" in dashed line for DESI (perhaps expected?) but also for SDSS

This is actually happening for all three of DESI, SDSS, & BOSS. It is showing an "other model" for a different spectrum, and it is probably related to the arrow-slider issue (item 3) identified above.

The arrow-slider synchronization appears to be working for every other spectrum. In between we get a "ghost" model for the last-displayed spectrum. If you look very carefully, and advance through the spectra, you should only see the "ghost" model on every other spectrum.

Before we submit a ticket, let's make sure gp12 has the latest version of Prospect. Currently it has 1.2.5, but 1.3.1 is the latest. Since gp12 is due for imminent rebuilding, this could take a while. In the meantime, perhaps you can test on a system that has Prospect 1.3.1?

weaverba137 commented 8 months ago

I've created desihub/prospect#98.

stephjuneau commented 8 months ago

OK, so I think that as far as this notebook is concerned, we could merge this version and separately follow the Prospect issue(s) such as in the new ticket you created. So I'll go ahead and approve this version now that you fixed the typo!

weaverba137 commented 8 months ago

I confirmed that the issue is still present in Prospect/1.3.1, so I don't think anyone else needs to try to re-confirm that.

rnikutta commented 8 months ago

Thank you for the fix @weaverba137 and for the review @stephjuneau ! Merged.