Tickets/preops 3512: Incorporate refactored pre-night briefing code from the ADACS team into schedview

[ehneilsen]

There were elements of the prototype that the ADACS code did not include (because it was added after they started work), and other parts of the code that they did include that were not needed, so ultimately reworking the prototype to follow Eman's example was not just a simple matter of tweaking Eman's code, but the result is much cleaner now after following her example structure and reworking as needed.

The structure may not make a lot of sense without some idea of how the param module works. See here.

[rhiannonlynne]

In general, adding more comments in the code would be helpful to understand how things are being used, or why they're being set up in a particular way (i.e. multiple methods watching the same parameter and doing a 1-liner, because you wanted one method per parameter, etc.)

Another place comments would help is in the definitions of some of the parameters you're using -- "night" seems to be used here differently than we use it in the scheduler outputs, for example (where it's an integer, counting from the start of the survey .. it's not obvious to me what it is here, but it seems like it's just the time?). And night vs. night_time (why have both?).
Similarly, 'observationStartMJD' vs. "start_date". I am unclear on why the new terminology is being introduced and if the difference is signifying something I'm not catching.

[rhiannonlynne]

Could use param.Filename here maybe .. result would show up in report with full pathname, which may be useful if multiple directories have the same opsim file?

[ehneilsen]

I considered param.Filename, but it won't work, because we want to support loading data from URLs, not just files on the user's laptop.

[ehneilsen]

@rhiannonlynne , I have added both comments and greatly expanded the use of param.Parameter declarations to embed documentation and validation. I think this should address your suggestions about expanding the comments.

[ehneilsen]

The automated tests are failing on my VM, but I think this is due to a bug in rubin_sim no longer being able to create an example scheduler with any nside other than the default. I hope this will be easy to fix in rubin_sim, but I can make a work-around in schedview (just test with the default nside) if that turns out to be necessary. But, using a smaller nside makes the tests run a lot faster, and lets me get away with using a VM with less memory, so I'd rather this get fixed by fixing rubin_sim.

[rhiannonlynne]

One of my concerns with the comments was more related to in-line comments about why things were being done one way (rather than another) and sometimes it’s hard to know where to place these comments, so I can see that it’s difficult. It’s also hard to know what someone might want to understand vs. what may be clear to the person writing it. Basically, if there are places where you’ve made conscious design choices that should be continued, or even if they should be reconsidered when we come to refactor this into its future homes, it’s likely useful to leave a comment in the code I think.

[rhiannonlynne]

Anyway, if you can sort out the rewards function panel, that would be good. There are places in the rest that feel a little clunky, when looking at the code, but that maybe are not .. and in general, I think this is a good place to accept the PR and then continue refinements later in different tickets.

[rhiannonlynne]

Nice! I was just trying to find out how to do this looking at the documentation, since I was thinking that the juggling between astropy.Time and datetime.time may not be necessary in the prenight app (if you made a Time parameter and used that, which would probably show up looking nicer on screen).

[rhiannonlynne]

Is this going to fail the param checking if the columns don’t match exactly? I am undecided if that is good or bad, given that we may change the scheduler outputs and this would then tie the scheduler + schedview versions … OTOH, maybe that is just something to consider about the refactoring and the columns here could be tied to the columns created by the scheduler when it’s run?

[ehneilsen]

It will fail if the provided DataFrame doesn't have these columns, but it will pass if it has extras. I think this is what we want, because these are the columns the dashboard needs to function, and the dashboard will happily ignore extras.

[rhiannonlynne]

so _night_time doesn’t necessarily equal “night” or does it? the fact that it’s described as the start of the night is throwing me off here. is “night” the actual time or the time at the start of the night? and is “night” a time or a (essentially integer) describing which night it is?

[ehneilsen]

It is technically a time, but it is used to designate a night, but that's true of all mechanisms for specifying a night that are reasonable in this context.

The rubin_sim approach (numbered night of the survey) doesn't work because we shouldn't be asking observers to keep track of when the survey starts, or make them calculate how far away it is. Similarly, we don't want to put this date on plots because this will then require the user to do the reverse of this to understand what they are looking at. This approach is ambiguous, and leads to multiple different nights with the same night (when the start date of the survey gets rescheduled). For auxtel, it will also be a negative number, which I suppose would work, but would be a bit weird. Ultimately, in the context of dashboards, what we really need is a calendar date because this is what users will want to specify and what they will want to see plotted. (Even in the context of the simulation, I've always found using the count relative to the start of the survey to be awkward to deal with, because you need to remember to add the start date to do anything useful with it.)

But, calendar dates have their own problems. The local night "rolls over" at midnight, and the UT night (in Chile) "rolls over" around the start of the night, sometimes before, sometimes after. So, the calendar date "July 11, 2023" is ambiguous: it might mean the night that begins at July 11, 2023 local time, or it might mean the night almost all of which happens during July 11, 2023 UTC (which is a day earlier than the one that begins on July 11, 2023 local time, and so the night that ends on July 11, 2023 local time).

So, to express the night, it needs to be a calendar date (so that a human can specify and interpret it), and it must include a time and time zone. The practice used at the NOAO (now NOIRLab) observatories I have worked with (CTIO and Kitt Peak) is to use the local date of the time of the start of the night. (SDSS used the UT of midnight, so was offset from NOAO by one.)

The panel input widget supports the python stdlib datetime.date widget, which is basically a tuple of year, month, and day. Nothing about the data type, though, indicates whether it should be interpreted as it would be at CTIO (when night that begins when the local date has these values) or as it would have been at SDSS (when UTC has those values at midnight). So, first, it needs to be documented.

Using an astropy.time.Time instance is an unambiguous way to specify a night, for any times we might care about: the night for a given astropy.time.Time instance is the night in which that instant in time occurs. (I specify "we might care about" because the time between local civil and astronomical noon is ambiguous, but we really don't care.)

Of course, as I write this, I noticed a pseudo-bug it how I converted the datetime.date to and astropy.time.Time. The current way I did it will always give the right answer in Chile, but might be conceptually less clear. I'll fix it, and put the most of the text here into a comment in the code.

[rhiannonlynne]

Thanks - I agree that counting from the start of the survey is awkward and not what we'd want to do, and also that we do need to define what "night" means, so spelling out that it is the calendar date in Chile at sunset is likely useful (and will avoid any confusion if engineering time means that the observing was planned to start after local midnight).

"night" doesn't change during the operation of this code, unless you're skipping ahead to the next entire night, right?

[ehneilsen]

For the pre-night? No. In production, I intend to make the default date the current day, and there usually won't be a reason to change it in the standard use case (preparing for a night of observing). There will be exceptions, though. For example, if a user is in the middle of downtime, and wants to see what will happen on the first night back, they will need to set the date to the first date back, not the current date.
In practice, what the value is used for is to determine which night to report astronomical events for in the table at the top, and what time range to include in the visits in the following plots.

[ehneilsen]

In the future, I expect that the fields getting data (opsim output, scheduler pickle, etc.) will offer options from an archive. The night field might be used to limit the options offered to simulations that cover the night specified.

[rhiannonlynne]

It might be nice if this could use arguments as well as or instead of the os.environment variables, but I see that there are no arguments being passed here at all so i’m not sure if they’re just not available somehow with how this runs?

[ehneilsen]

This is a good idea, but maybe not for this ticket. I just created a new one for it: PREOPS-3600.

[rhiannonlynne]

Oh just noticed -- you may want to remove the version.py file that is in the repo, so that version.py can be written by setup tools_scm?
Maybe also the old copy of the environment.yaml file?