LSST-nonproject / sims_maf_contrib

Contributed code for MAF (sims_maf)
18 stars 46 forks source link

Notebooks updated for v3/v4 and python 2/3 #53

Closed yuchaz closed 6 years ago

yuchaz commented 6 years ago

Fix the following notebooks.

  1. Complex Metrics.ipynb
  2. Dithers.ipynb
  3. MAFCameraGeom.ipynb
  4. PullLightCurves.ipynb
  5. Slicers.ipynb
  6. Stackers.ipynb
rhiannonlynne commented 6 years ago

Hi @yuchaz I had a request from a user to update the PullLightcurves notebook, so I did that one last week (sorry - didn't know you were working on it too). Can you have a look at what I did there? (It's on master) I think it was somewhat useful to rearrange a few things and add the cell that highlights the difference between v3 and v4, some of which may be useful for you to carry forward for other future work. I'd rather use that version than the one here, which makes the PR a bit more difficult to merge. Perhaps you can pull in the sims-maf-contrib master version of the PullLightcurves notebook and update the PR?

Some other notes - please add "from future import print_function" to the head of the notebooks, as we may still have people using py2 for a while (although they will be expected to have future, from the LSST python installation). You don't have to add the #changelog notes to the notebook cells, as we intend these for users to go through as examples (and these comments may be confusing for someone coming in and seeing the notebook for the first time) -- adding them to the git commit message would be nice though.

Can you update "runName" to be the same as the opsim database name? In some of the notebooks (ComplexMetrics, for example) the run name is only set in the metric bundle, and not earlier, and so it's easy to get out of sync. It would be good to change the notebook to set this earlier. (if you notice, the runName goes into the plots, and so records what opsim database/ opsim run the plots reflect -- this should be consistent with what the run actually is).

I'm a little puzzled about the multiple chip warning in the CameraGeom notebook - I don't think this should happen and will check with the sims camera geometry package maintainer about the error message. Before I go too far there though, can you please try running the notebook again with Owen's newer docker image (sims 2.5.0)? I'm kind of hoping it's a bug that got fixed.

This is a great start!

yuchaz commented 6 years ago

Thank you @rhiannonlynne I will edit them :) . And sorry for the PullLightcurves notebook issue. I should have update my forked repo before creating the pull request

yuchaz commented 6 years ago

Hi @rhiannonlynne I fixed the notebooks. As for the the multiple chips warning in CameraGeom.ipynb notebook, it's still there even if I ran with latest MAF (version 2.5).

rhiannonlynne commented 6 years ago

Hi @yuchaz thanks!

I think I may have been confusing when I was using the OpsimDatabase.opsimVersion information before. Clearly, we should probably write up a little summary of the differences between v3 and v4, which would be helpful. There are also changes between older versions of MAF and the current version, which have been added to handle both V3 and V4 versions of the Opsim output databases. So there's a lot going on! The notebooks need to update from old MAF calls to new MAF calls (the primary thing here is the availability of new kwargs and default values, and moving the sqlWhere from a function in utils to a method on OpsimDatabase). They also need to update to handle the new OpsimDatabase outputs (V4 instead of just V3). These differences are column names, but also that all angles in v4 are in degrees instead of in radians in v3. These changes can be accomodated by updating the kwargs, and using a kwarg set to indicate degrees or radians.

I realize now that this also makes the rest of the notebooks much more complicated. I guess it's good to show the differences, but if you'd prefer to leave everything in the V4 default mode, I think that would be perfectly reasonable as well (in which case, a lot of the comments below here you can ignore) -- but then it'd probably be good to remove the v3/v4 option as well.

So this will seem picky but here's how including the v3/v4 differences affects the updated notebooks:

yuchaz commented 6 years ago

Hi @rhiannonlynne I just fixed what you pointed out. Let me know if there's any other things that need to be fixed.

rhiannonlynne commented 6 years ago

Looks pretty good, just one more problem with the Dither notebook. Could you either take out the V3 options (i.e. setting the expMJD/degrees for v3 & v4) or add explicit stackers for the dither stackers (like you did in the MafCameraGeom notebook, cell 8)? As it stands currently, the notebook wouldn't work for a v3 database because the stackers don't have the fieldId column + the angles are in degrees -- here's an example v3 database so you can try it out! http://astro-lsst-01.astro.washington.edu:8081/astro/store/pogo4/db_gzip/minion_1016_sqlite.db.gz)

yuchaz commented 6 years ago

Hi @rhiannonlynne I just fixed Dithers.ipynb. However, there are two things I would like you to ask/check.

  1. After upgrading the version of MAF to 2.5, some of the API changes therefore cell 8 will fail. I ask @oboberg and he suggested me this quick fix.
  2. In this version, I use v3 dataset. And the output in cell 22 is sparse. Is that normal?
yuchaz commented 6 years ago

Hi @rhiannonlynne I just updated all the notebooks and the description texts on Index. I also did the sphinx documentations but I think I should separate it with different PR and create it when all the descriptions are revised. Beside this, I have several questions to ask:

  1. In cell [8] of VectorMetrics.ipynb, I change the col in metrics from finSeeing to seeingFwhmEff. Is that the correct way to change it? I cannot find the description of finSeeing since it's not even exist in v3 dataset.
  2. In cell [10] of Spatial_Coordinates.ipynb, I changed the ditheredRA and ditheredDec to fieldRA and fieldDec if its v4 dataset. (because the SummaryAllProps table in v4 dataset does not have dithered* columns. But seems like its redundant for v4 because the output is the same as cell [8].
  3. The lower third figure in cell [16] of HybridCameraGeomStudies.ipynb does not show anything. However, I used the same sql constraint as that used in ChipNames.ipynb. Therefore, the result should be the same as cell [8] of ChipNames.ipynb. Do you know how to solve it?
rhiannonlynne commented 6 years ago

Hi @yuchaz sorry for the delay. Owen is going to deal with more detailed comments, but I wanted to add: