PREOPS-4646: feature- Support addition of opsim data to an archive for use by schedview-prenight #20
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 88 90 +2
Lines 8843 9125 +282
======================================
- Misses 8843 9125 +282 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
There are some things I don't understand the need for or alternatively the implications of, but comments are in the code. This doesn't mean that the changes shouldn't be there, but I don't understand it well enough and a little more explanation would be helpful.
| @@ -30,6 +30,7 @@ jobs: | |||
| run: | | |||
| mamba install --quiet --file=requirements.txt | |||
| mamba install --quiet pip | |||
| pip install lsst.resources | |||
There was a problem hiding this comment.
We should get this into conda-forge ...
but I'm wondering if we can't add it to the requirements.txt file anyway?
If it's not necessarily for most uses, but only for the ones we're in control of for now, this is probably fine.
There was a problem hiding this comment.
Currently, requirments.txt is just a simple text list of packages, and I don't think this format supports pip/pypi packages. Such packages are supported by the conda's yaml format for specifying environments, and perhaps we should move to that format, but maybe that should be done as a separate issue (and corresponding PR).
| @@ -12,3 +12,4 @@ h5py | |||
| requests | |||
| shapely | |||
| tqdm | |||
| conda | |||
There was a problem hiding this comment.
Can you explain why we need conda as a requirement in a file that is installed with conda?(I'm not following)
There was a problem hiding this comment.
One of the bits of metadata that can be included in the archive is the specification for the conda environment that ran the simulation. To get the envirenment, the python code uses the python api to conda, and so the conda package proper must be present.
There was a problem hiding this comment.
You mean you are in a situation where this code is not running in a conda environment but needs to get information from a conda environment elsewhere?
There was a problem hiding this comment.
No, the code is running in a conda environment, and we want to record the details of that environment as metadata along with the results of a simulation run using code in that environment, to make it easy to answer questions like "What version of X was being using in the process that produced output Y?"
There was a problem hiding this comment.
So in that case conda is already installed in your conda env and you don't need pypi to install it. I don't think it should be listed as a formal dependency but should have an import protection around it like we do in lsst-utils package when retrieving conda package versions.
There was a problem hiding this comment.
It's in the conda base environment, not the environment that is actually running the code and writing it to the archive. For example:
(schedview) [neilsen@sdfrome002 rubin_scheduler]$ python
Python 3.11.6 | packaged by conda-forge | (main, Oct 3 2023, 10:40:35) [GCC 12.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from conda.cli.main_list import print_packages
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'conda'
There was a problem hiding this comment.
Yes. That's why we add conda to rubin-env. Does adding it to the formal dependencies cause problems when pip installing?
There was a problem hiding this comment.
Since we have a pyproject.toml file with project dependencies, I think you shouldn't have a problem with pip installation. I guess if conda is part of Rubin-env, it's reasonable to add as a dependency for this package.
| survey1.set_script(obs_array[all_other]) | ||
|
|
||
| survey2 = ScriptedSurvey([bf.AvoidDirectWind(nside=nside)], detailers=euclid_detailers) | ||
| survey2 = ScriptedSurvey([bf.AvoidDirectWind(nside=nside)], nside=nside, detailers=euclid_detailers) |
There was a problem hiding this comment.
The addition of nside here -- this seems outside the range of the "add archiving to the simulation outputs" ... was this a case of something that went missing from the example_scheduler?
(I ask because I think we're in a little bit of a race condition between example scheduler in rubin_scheduler and example scheduler in the sims_featureScheduler_runs repos, and we need to communicate super clearly about changes that might be needed).
There was a problem hiding this comment.
One of the very recent changes to rubin_scheduler (which added the direct wind basis functions to scripted surveys) caused schedview's tests to fail because the nside of the survey in the example scheduler in the didn't match the nside of its basis functions. (The schedview unit tests set a smaller nside to make the test go faster.)
So, this fix was needed to make schedview's tests pass after the rebase before the pull request, although really it's a fix for a bug in the pull that got applied in the rebase.
There was a problem hiding this comment.
I pulled that change over to sims_featureScheduler_runs3.4. Also added it to survey1 above since it looks like that should need it too.
|
|
||
| night_index = matching_nights.item() | ||
|
|
||
| return night_index |
There was a problem hiding this comment.
It would be helpful to know a bit more about where this is being called from. It seems like it's the same (at the end of the day) as mjd_index, but with the added flexility of letting you choose any longitude (any site). I'm not sure I understand allowing variable longitude, since the Almanac is only created for the Rubin site.
mjd_index should be returning the same thing as index_for_local_evening, if you convert local evening time into MJD UTC, but I can also see how the index_for_local_evening lets you do things that don't make sense (like using a longitude that doesn't match our almanac), and the utility of that isn't clear to me.
Also, since rubin_scheduler only uses index_mjd internally, it's not obvious to me what this does and how it works with scheduler output.
So I'm confused.
There was a problem hiding this comment.
mjd_index "rolls over" at midnight UTC, which sometimes happens during twilight, in which case there can be a few visits at the start of the night where mjd_index and index_for_local_evening can be different. Using index_for_local_evening just makes sure "nights" correspond to actual local nights, rather than UT dates.
Actually using the site longitude was mostly just pedantry on my part. I could just as easily have subtracted an hour or two, and it would have been sufficient.
There was a problem hiding this comment.
Looking more carefully at it, it looks like sunset is always before the rollover (if sometimes only by a few minutes), so yeah, they should be the same. So, I was just being overly careful in this case, thinking that sunset was sometimes before the rollover, sometimes after. (I've been bitten by this often enough that I've learned to pay attention to it, and just always use the longitude to shift the rollover to noon or midnight.)
There was a problem hiding this comment.
How does this relate to the observing concept of DAYOBS which is 12 hours offset from UTC. Should your concept of night and the summit's concept of night share the same definition?
There was a problem hiding this comment.
I'm not familiar with DAYOBS, but a simple shift of 12 hours results in a rollover that is always just after sunrise (but sometimes very close to it), so the entire night (sunset to sunrise) will be on the same date. In this context, we're only talking about dates (YYYY-MM-DD or integer MJDs), not date-times, and as long as the whole night (sunset to sunrise) has the same date, I think it's equivalent.
The only ambiguity is the direction of the shift: is the date of the night the date of the evening or of the morning? We've been using the date of the evening, but I've seen other observatories use date of morning, so there's the possibility of an off-by-one problem. I did ask around when I first started working on Rubin observing stuff, and was told that, like other NOAO (at the time) observatories, it uses local date of evening for the date of the night, but my discussion was never at formality level of "this is how DAYOBS is defined", or even that it was called DAYOBS.
There are other contexts where a difference might be important (e.g. assigning visits taken when the sun is up to dates), but that can't ever be relevant to this class,.
There was a problem hiding this comment.
See the definition of dayObs in https://sitcomtn-032.lsst.io
This is what the camera uses to determine what YYYYMMDD to use for the next data file it is taking. Butler can be queried using that day_obs.
There was a problem hiding this comment.
From SITCOMTN-032:
dayObs A natural number representing the observation day (in timezone UTC-12:00)
So, it does look like the earlier (evening) date is the one used, so it should always match.
There was a problem hiding this comment.
Yes, I was wondering whether to bring up dayobs .. Merlin has a few utilities related to this that might be useful to know about (such as adding dayobs values) although I can't quite remember where (probably in the summit utils repo ? https://github.com/lsst-sitcom/summit_utils).
One of the things Peter recently did was make sure that index_mjd gave the "correct night" related to sunset, especially when using sunset rather than 18 degrees sunset.
If these are the same, can we drop the local evening version? And perhaps make it clear that index_mjd should return the correct "night" compared to sunset? (I kind of prefer the "local evening" naming, because it's really clear what's being calculated, but index_mjd could just have this clarification added).
There was a problem hiding this comment.
I think that dayobs is probably useful to know about, but not directly useful for our particular use-case ... my understanding is that we're using a datetime.date value instead of MJD primarily for the date-picker for schedview, and inside schedview converting between datetime.date (for the date picker widget) and astropy.time (for MJD calculations and simulations). rubin_scheduler should probably be primarily concerned with astropy.time and MJD values, although it's possible for simulating an upcoming night that dayobs might be useful (and here I say dayobs just so that it is clear it matches up with summit and DM expectations). And maybe schedview knowing about dayobs would be good, just to help make it clear that we're all talking about the same thing again.
Schedview could use dayobs and recast into datetime.date if the picker widget is still useful maybe?
rhiannonlynne
changed the title
This pull needs to be coordinated with the corresponding PR on schedview.