lsst / rubin_sim

Scheduler, survey strategy analysis, and other simulation tools for Rubin Observatory.
https://rubin-sim.lsst.io
GNU General Public License v3.0
41 stars 37 forks source link

handle constant weather #371

Closed ehneilsen closed 12 months ago

ehneilsen commented 1 year ago

This is the first part of PREOPS-3847, the other part being a change to schedivew

rhiannonlynne commented 1 year ago

I guess this is the first time we have a data class returning more than one value at a given time, naming the tuple type seemed odd but fine. Did you check that this wind direction definition is how T&S is expecting to define wind direction? There is already a wind avoidance mask that we would want to make sure this matches https://github.com/lsst/rubin_sim/blob/fb6358b5328eb7c617116b814742e9950bfe57e4/rubin_sim/scheduler/basis_functions/basis_functions.py#L1874 Usually we define user-facing functions with degrees instead of radians, but if the incoming weather info is in radians, we should match that instead (and maybe give a note about why the deviation).

ehneilsen commented 12 months ago

I think this is fine, but want to ensure we're matching existing expectations from telescope and site regarding wind direction. (I think this is likely true, but telescope and site probably ought to be passing us wind direction in degrees and I'm a little surprised it seems otherwise).

I was carefully following the documentation of wind_speed and wind_direction in the documentation of the Conditions class (lines 136-141 of conditions.py) for the units and meaning of wind speed and direction. (I note that, based on the git commits, @tribeiro wrote this documentation, so I expect that it's what T&S want.) If it had been up to me, I would have preferred direction in degrees as well.

In production, I expect that they are passing us wind speed and direction, just as I expect that they are passing the seeing. But, rubin_sim needs a mechanism to set it when used for purposes other than actually scheduling real visits (e.g. to get schedview to tell the user what will happen under different circumstances).

ehneilsen commented 12 months ago

Looking at the use of conditions.wind_direction in the AviodDirectWind basis function:

wind_pressure = conditions.wind_speed * np.cos(conditions.az - conditions.wind_direction)

it certainly looks like it needs to be in radians, because numpy's cos function takes radians.

ehneilsen commented 12 months ago

Stylistically, I like using namedtuples when returning multiple values from a function. They are self-documenting like dictionaries or dataclasses, but are as convenient as tuples (because they are tuples).