tickets/SP-3225: Add per-night observing statistics to report link table and RSS feed#206
Conversation
…am prenight rss entries
| ) | ||
|
|
||
| # Filter to the requested day_obs. | ||
| if "dayObs" not in all_visits.columns: |
There was a problem hiding this comment.
Isn't this function only for getting the consdb data?
There was a problem hiding this comment.
The function caches the return of read_visits or read_ddf_visits, both of which (optionally) apply stackers to the return of consdb queries, and in particular this function uses the column made by the dayobs stacker to figure out which visits to return, because the cache contains all visits by the caller might have requested a limited range of dayobs.
That the user might have requested different stackers than the call that filled the cache means the cache isn't valid, though, which is why it saves a df listing the stackers used to make the cache.
| except ImportError: | ||
| SCIENCE_PROGRAMS = () | ||
|
|
||
| _BANDS: tuple[str, ...] = tuple(ModelObservatory(no_sky=True).bandlist) |
There was a problem hiding this comment.
It would be helpful to have a comment here to know what _BANDS is intended to do later. it's .. not obvious.
And if it's really instantiating a ModelObservatory just to get a list of ugrizy ... I think it'd be better to just specify ugrizy.
(was this Claude?)
There was a problem hiding this comment.
If I remember the devel history correctly, I think the original implementation (written by claude code using a notebook I hand wrote and the design doc as context) just assigned _BANDS to list('ugrizy'). Using a different LLM to do a code review (GPT codex IIRC), one of things it suggested is importing from a the band list from central location where it could be reused. It is completely correct: as a general rule, it is good practice to do this, but conceptually schedview is the wrong home for this. ModelObservatory is the proper home for this, so I made the change to using that, thinking that the LLM was right, that it is best practice (but probably not one that matters much in this case).
There was a problem hiding this comment.
I've set it back to
_BANDS: tuple[str, ...] = tuple("ugrizy")
There was a problem hiding this comment.
Mostly my issue with using ModelObservatory is that it's a pretty heavyweight class to be using for such a lightweight thing as this.
You could use Syseng_values if this would be better?
Presumably if we do add an "u_v2" or something, it would show up in syseng_values and then you (and the other LLM) are right, we wouldn't have to rewrite everything.
Although you could also imagine the bandpass list to change enough that it breaks in other ways, by having an underscore instead of a single character.
There was a problem hiding this comment.
I've already checked in the change to turn it back to a tuple declared here, so for the PR I think we should just leave it that way.
For a more long term refactoring, I think we should just make bandlist a class attribute of ModelObservatory instead of an instance attribute, so we have access to it without needing to instantiate ModelObservatory. I think there are other similar attributes of and methods of ModelObservatory that we can make class attribute of methods as well. This seems like the clean solution to me, but will require a change to rubin_scheduler rather an just schedview, so belongs in a separate issue & PR.
This PR adds basic summary statistics to
make_report_link_tableandmake_report_rss_feed.The new
schedview.compute.smallsumsubmodule actually does the statistics computation itself.The new
schedview.collect.visits.cached_read_visitsfunction manages a local HDF5 cache so the ToCgeneration does not need to requiry consdb for the full set of visits if such a query has been done
recently to generate the nightsum (which will generally be the case).
I have also used this PR as a test case for learning to use LLM coding tools more effectively, using a variety of different coding tools backed by a variety of different LLM models, including
claude code,cline(a vscode-integrated tool),ellama(an emacs integerated tool) backed by LLM model APIs provided by SLAC, Fermilab, or running locally on my laptop (claude-opus, claude-sonnet, gpt-5.3-codex, qwen3-coder-next, gemma4-27b).There are also design documents for these changes in the
design/SP-3225directory.I've been experimenting with using design documents as a way of providing context to LLM tools, and to have LLM tools document a design for the code it generates.
Before letting the LLM tool implement code, I provide it context (goals, restrictions, etc.) in the design document, then have it add its design to the document, and iterate until I'm happy with the design. Then, I prompt it to implement what is written.
In practice, the process can go the other way to: to get the code working the code usually ends up being either human or LLM edited in ways that contradict the original design, in which case I prompt it to update the design to match the code.
The design documents include code examples, the point of which is to provide a human confidence that the code as-written actually matches the document when
doctestcan verify them.I added some new testing configuration to support this. If you run:
then
pytestwill run doctest on.rstfiles in thedesigndirectory, and also verify that the.rstis formatted correctly.If the
-m designoption is not used, these tests should be skipped.Note that GitHub renders
.rstfiles nicely in its web interface, so you can see these documents nicely formatted if you look at them that way.