Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle constant weather #371

Merged
merged 1 commit into from
Oct 10, 2023
Merged

handle constant weather #371

merged 1 commit into from
Oct 10, 2023

Conversation

ehneilsen
Copy link
Collaborator

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

@rhiannonlynne
Copy link
Member

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

class AvoidDirectWind(BaseBasisFunction):

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).

Copy link
Member

@rhiannonlynne rhiannonlynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@ehneilsen
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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).

@ehneilsen ehneilsen merged commit 62f7260 into main Oct 10, 2023
6 of 9 checks passed
@ehneilsen ehneilsen deleted the tickets/PREOPS-3847 branch October 10, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants