Classes

[armengau]

• Restructured viewer.py with classes (ViewerCDS, ViewerPlots, Viewer[VI]Widgets, ViewerSetup).
• Code tested both in test notebooks and with scripts generating html.

Ready to merge on my side.

[coveralls]

Coverage decreased (-0.05%) to 1.449% when pulling ee8e009 on classes into 7e00380 on master.

[weaverba137]

I'm taking a look, but one initial comment. Why choose to make viewer_cds.py, viewer_plots.py, viewer_setup.py... when you could put those all into a subpackage: viewer/cds.py, viewer/plots.py, viewer/setup.py?

I also updated the api documentation.

[armengau]

I'm taking a look, but one initial comment. Why choose to make viewer_cds.py, viewer_plots.py, viewer_setup.py... when you could put those all into a subpackage: viewer/cds.py, viewer/plots.py, viewer/setup.py?

The rationale is: each file has a name close to its associated class eg ViewerCDS, ViewerPlot etc.

I'd rather avoid class names "CDS", "Plot", etc, such names would be too generic, too easy to be confused with.

On the other hand, I fully agree to have eg class ViewerCDS implemented in viewer/cds.py, if you think it's better.

[weaverba137]

Indeed. When you use names like viewer_plots, you're just reinventing the wheel on top of the namespace facility Python already provides via viewer/plots. However, you should still name the classes e.g. ViewerCDS.

[weaverba137]

In prospect/scripts/specview_per_expo.py:

from ..utilities import match_zbest_to_spectra, match_vi_targets  #, miniplot_spectrum

There is no such function match_zbest_to_spectra() in prospect.utilities.

[weaverba137]

I found an apparently misplaced class, but overall I think this looks fine. We just need to do a full test with data on various systems. I should be able to test on Data Lab with SDSS data later this week. There are some problems with that system right now.

[weaverba137]

PS, Let's not attempt the rearrangement viewer_cds.py --> viewer/cds.py right now. Let's concentrate on testing the branch as-is, and then if everything works we can rearrange or save that for another PR.

[armengau]

In prospect/scripts/specview_per_expo.py:

from ..utilities import match_zbest_to_spectra, match_vi_targets  #, miniplot_spectrum

There is no such function match_zbest_to_spectra() in prospect.utilities.

Indeed. This script specview_per_expo.py is outdated since early cmx - but I won't update it in this branch.
Fixed in 695181c

[weaverba137]

OK, let's move to testing on DESI & SDSS data next.

[armengau]

I redid the tests:

• on a small test notebook
• running the same script as to create a subset of blanc VI pages (https://data.desi.lbl.gov/desi/spectro/prospect/dev/test/tiles_QSO/80605/deep/)
• on two notebooks in doc/nb, which I just updated (Prospect_demo.ipynb and Prospect_targetid.ipynb)

Looks ok for me.

If you agree I would prefer to rearrange the dir tree, eg viewer_cds --> viewer/cds, in this PR. Btw, where would you locate viewer.py ?

[weaverba137]

viewer.py can become viewer/__init__.py. There are slightly more complicated things you can try, but if you want to test the rearrangement, start with that.

I'll test on Data Lab as soon as I can.

[armengau]

Last commits 53b2c88 : changed the dir tree as suggested.
I also edited "Setup" classes to "Layouts", to avoid having a setup.py file...

Tested on notebooks+scripts, ok on my side.

[weaverba137]

I tested on Data Lab this morning and found no problems. Merge when ready. After merge I will create a new tag.

[weaverba137]

I also added an update to one notebook.