Obs wafer band in obsdb #1159
Obs wafer band in obsdb #1159
Conversation
|
I like this idea! I go to site and cannot review much for a few weeks. |
There was a problem hiding this comment.
This is totally the right direction for obsdb, thanks for figuring it out.
Obviously this needs tests and stuff :) so there will be a few rounds of iteration. But I'm on-board with the concept.
You've hard-coded the extension to OWB, which makes sense for SO. But it's a small step to fully generalize it for more arbitrary index combos. So I might push for that in next iteration. (The "get_primary_keys" function shows that you can figure out the obs_key for a given database.)
sotodlib/core/metadata/obsdb.py
Outdated
| warn_str = 'WARNING: Primary key(s)' | ||
| for i, wb in enumerate(wafer_band): | ||
| if wb is None: | ||
| wafer_band[i] = '_all' |
There was a problem hiding this comment.
This failed for me because I passed in a tuple. Also it's not great to modify someone's inputs if you don't need to. Instead, start the func with wafer_band = list(wafer_band); then you'll definitely be able to change it and return it.
Ok @mhasself I think I've addressed all of your comments, please have another look when you can. Thanks. |
sotodlib/core/metadata/obsdb.py
Outdated
| query = "PRAGMA table_info('obs')" | ||
| c = self.conn.execute(query) | ||
| primary_keys = [row['name'] for row in c.fetchall() if row['pk'] > 0] | ||
| return primary_keys | ||
|
|
||
| def warn_primary_keys(self, wafer_band): | ||
| def warn_primary_keys(self, wafer_info): |
There was a problem hiding this comment.
Make private. (def _warn_primary_keys)
Extend this so that wafer_info can be a dict (in which case the needed values are extracted and returned to your caller in the desired format).
There was a problem hiding this comment.
@mhasself what structure are you thinking that users could pass in a dict for this wafer_info?
Like:
{`wafer_slot':'ws0', 'wafer.bandpass':'f150'}
And I would return ['wafer_slot', 'bandpass'] or something like: {wafer_info:['wafer_slot', 'bandpass']}? I guess I'm not really sure what the useful aspect of the values in the key-value pairs of a dict would be in this case.
sotodlib/core/metadata/obsdb.py
Outdated
| obs_key = {'obs_id': obs_id} | ||
| if (len(self.primary_keys) > 1): | ||
| wafer_info = self.warn_primary_keys(wafer_info) | ||
| for i, k in enumerate(self.primary_keys[1:]): | ||
| obs_key[k] = wafer_info[i] |
There was a problem hiding this comment.
Here, and elsewhere, we should take this opportunity to improve the interface.
For backwards compatibility, indeed we need to accept the first obs_id as a string; thus it makes sense to accept the wafer_info as more strings... But it would be helpful to also accept "obs_id" as a dict containing all the primary key fields. (And, as mentioned earlier, it would also be helpful to accept wafer_info as a dict.)
Normally I'd do this with a little validation+conversion function that takes the user-provided args and turns them into the most convenient form. i.e.
obs_key = self._construct_obs_key(obs_id=obs_id, wafer_info=wafer_info)
You could merge that functionality into your warn_primary_keys function.
There was a problem hiding this comment.
I see this is also a similar comment about generalizing to a dict structure as you added under the warn_primary_keys function. Can you please write out an example of what the obs_id as str + wafer_info as dict, or obs_id as dict + wafer_info as list of str or obs_id as dict and wafer_info as dict, etc. would actually look like?
There was a problem hiding this comment.
Right, so you already support this:
obsdb.update_obs('obs_2345_xyz_110', wafer_info=('ws0', 'f090'), ...)
I think it's advantageous to also support:
obsdb.update_obs('obs_2345_xyz_110', wafer_info={'wafer_slot': 'ws0', 'bandpass': 'f090'}, ...)
and
obsdb.update_obs({'obs_id': 'obs_2345_xyz_110', 'wafer_slot': 'ws0', 'bandpass': 'f090'}, ...)
and maybe even
obsdb.update_obs(('obs_2345_xyz_110', 'ws0', 'f090'), ...)
There was a problem hiding this comment.
Ok I've allowed these ways to pass in obs_id and wafer_info and this PR is again ready for review.
This is an update to obsdb to allow storing information keyed by obs-wafer-band without breaking the current obs-id primary key behavior and allowing us to retrieve common entries for an obs-id between 2 databases that could be keyed by obs-wafer-band or obsid.