Commit initial package, with unit tests and example.

[rhiannonlynne]

There are some thoughts I'll just leave here for the future:

timeHandler probably needs to be in some lightweight package of opsim utilities that are easy to import, if we're tied to it here (for unit test purposes, at least).

What is the real use of timeHandler here anyway? In the future, I think we need to move to seeing databases that are tied to actual times, because seasons matter. So a class that just tracks "time since the start of the simulation" may not be quite what we need or want. Also, we maybe need some extra restrictions on SeeingData to make sure seasons are matching to season unless it's part of timeHandler somehow?

[rhiannonlynne]

Otherwise: SeeingData read the seeing database and gives you FWHM 500 at any requested time. Note that small change here to allow for the seeing_db not to start at time=0.

SeeingModel translates fwhm500 into fwhmgeom and fwhmeff at any airmass(es) and for the filter wavelengths defined on init.

SeeingSim combines the two so that they're easy to use with the current scheduler code, and provides a backward-compatible API (calculate_seeing). Configuration would still need to be modified in the scheduler/socs code unfortunately, but that's somewhat unavoidable given that this class used to be in socs combined with a lot of other things.

[rhiannonlynne]

Oh, and there's a notebook in examples which demos the use of SeeingSim and estimates the approximate time requirements for use with sims_featureScheduler.

[andrewbheyer]

Ok so 2 comments,

• minor bug in the tests line 50 in test_seeingData
• By requiring the use of sims_photutils requires 4 other packages to be used that running OpSim otherwise would not need (sims_sed_library, throughputs, utils, sims_maps). I have made the changes onto 1 commit that is on your branch that I can either push, or create a new branch for. The changes are minor and rather than raising an error raise a warning and use default values that is similiar to what SOCS used.

[rhiannonlynne]

Ok, push to the branch? Opsim already requires utils because it requires sims_utils. On Mon, Apr 2, 2018 at 4:21 PM Andrew Heyer notifications@github.com wrote:

Ok so 2 comments,

• minor bug in the tests line 50 in test_seeingData
• By requiring the use of sims_photutils requires 4 other packages to be used that running OpSim otherwise would not need (sims_sed_library, throughputs, utils, sims_maps). I have made the changes onto 1 commit that is on your branch that I can either push, or create a new branch for. The changes are minor and rather than raising an error raise a warning and use default values that is similiar to what SOCS used.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lsst/sims_seeingModel/pull/1#issuecomment-378078922, or mute the thread https://github.com/notifications/unsubscribe-auth/ABxQ38LCcUlyhyuYhVzP2Tobf3r8BCbmks5tkrJjgaJpZM4S906q .

[tribeiro]

The main problem is not sims_utils but rather sims_photUtils, which then adds four new dependencies not required by OpSim. I agree with @andrewbheyer that it would be better to have a standard behavior that does not require the extra packages.

I'll add some comments to the last commit but, for instance, the dependency on sims_photUtils was not removed from ups table.

[danielsf]

For what it's worth, I am working on a refactor of the other sims package that will remove the dependency on sims_sed_library and sims_maps from sims_photUtils. If we decide to adopt that, you will be able to depend on sims_photUtils without bringing in the data-only packages.

I will run those branches by Lynne when they are ready (probably this afternoon).

[rhiannonlynne]

Regarding sims_photUtils: I am also going to recommend that we do not use the Cm values (used for calculating m5 values) in sims_utils without at least checking that they are up to date. Often we run opsim with out of date values, without realizing it. We need better version tracking there.

[rhiannonlynne]

My take on the sims_photUtils dependency is this:

• if you don't use sims_photUtils & throughputs to calculate the effective wavelengths and m5 (Cm) values, then you need to update the values you're using every time there is an update in throughputs. (and you need to actually remember to do that). And then you need to be sure you track which numbers you used (and ideally, point to which version of the throughputs + readnoise values they correspond to).
• If you do use sims_photUtils & throughputs, then you just record which version of sims_photUtils and throughputs you used, and you're always up to date (or at least consistent with a particular set of these versions).

The first model here breaks fairly often. If you decide to go this route, I think you at least need a unit test that pulls in the appropriate packages & checks that your default values are up to date. For example, if you're using the precalculated sky brightness values, you need to check that you're using a consistent version of the throughputs and m5 values throughout. A lot of the time these changes are small enough that you can get away with slight mismatches, so it's not disaster .. but it's not great, and it's fragile in case of bigger changes.

[rhiannonlynne]

Another thing to consider is: is this additional dependency really a big deal? Yes, adding dependencies is not great. However, MAF already depends on sims_photUtils, so most likely you will already be having this package available if you're using the same system to run both opsim and MAF. So, practically speaking, is this actually a problem? Note that there are ways around using sims_photUtils: you can define the filters yourself, and the unit test does not have to be run. So the package is perfectly runnable without sims_photUtils.

[tribeiro]

Ok, you got me on "The first model here breaks fairly often"...

[rhiannonlynne]

I was actually more thinking about the m5 values that were in opsim 3.61 one of our old baselines. After the fact, I could never quite figure out how we'd figured out what the m5 values were.

[rhiannonlynne]

But yes! 😉 recent examples exist too