DM-35941: Remove use of implicit dimensions in Prompt Prototype #25
Conversation
There was a problem hiding this comment.
I'm still slightly grumpy that we can't use constant things like instrument and skymap as butler defaults, but I think I understand why you changed it to this. My grumpiness shouldn't prevent merging; this looks fine otherwise (note a few comment-related comments).
Should we just be storing instrument.getName(), or do we need other properties of the Instrument object?
| collections=self.instrument.makeUnboundedCalibrationRunName() | ||
| ) | ||
| self.skymap = self.central_butler.get("skyMap") | ||
| # TODO: is central_butler guaranteed to have only one skymap dimension? |
There was a problem hiding this comment.
Given the assert below, I think you don't need this TODO. We're defining the central repo as having exactly one skymap as part of this, and I think that's something that we as the AP group can ensure.
There was a problem hiding this comment.
I don't see why the assert makes the TODO irrelevant. The assert is computer-readable but easy for a human to overlook.
I'm not so confident that we will only have one skymap -- I can imagine a lot of experimentation over which skymap gives e.g. the best performance, especially during commissioning, and there might even be a policy some day that different datasets or data sources are based on different skymaps. Ultimately, it's not our call, but K-T's and the commissioning team's.
But the problem was that these things aren't constant, especially when the local repo is first being set up -- the code made assumptions that weren't actually true IRL.
Well, we use a lot of |
|
Name to instrument is standard API. We use it all the time for the command-line tooling: |
kfindeisen
force-pushed
the
tickets/DM-35941
branch
2 times, most recently
from
63b2cae to
ca496da
Compare
The new code avoids keeping around a tuple when we only want one component.
The meaning assumed by upload.py and activator.py is taken as truth; tests have been updated to match.
Coadded templates are band-specific; the previous query pulled templates for all bands even though only one was needed.
All queries already used full dimensions, so this is just a matter of removing a few comments.
This avoids bugs related to the inconsistent and idiosyncratic handling of governor dimensions inside Middleware.
This avoids bugs related to the inconsistent and idiosyncratic handling of governor dimensions inside Middleware.
This change protects the code from depending on defaults in the future.
The function requires that all queries be expressed in a way that is valid (but potentially returns nothing) for both central and local repositories.
kfindeisen
force-pushed
the
tickets/DM-35941
branch
from
ca496da to
28b2f97
Compare
3 participants
This PR adds explicit
instrumentandskymapdimensions to all Butler queries, and removes code that the use of default dimensions had made necessary. The result is much more likely to behave consistently in different conditions, increasing the value and relevance of the existing unit tests. The first few commits fix some smaller bugs discovered while examining the code or testing the explicit dimensions.This PR depends on, and cannot be merged before, #24.