DM-13685: Add storage of calibrated fluxes and computation of flux means to ap_association #15
Conversation
Fix bugs in store_dia_sources Debug test_association_db_sqlite Add handling of nan values to record test. Flesh out test_store_ccd_visit_info. Debug test_association_db_sqlite
Debug test_association_db_sqlite Fix psFluxMeanErr name Fix typos
Fix aliasMap bug Add test for correct schema values in run. Partially debug test_update_dia_objects
Fix bug in update_dia_objects
Fix test comparison numbers Finish unittests for association_tack Add and edit doc strings and comments.
| Force the value of diaObjectId for a DIASource. | ||
| overwrite_dict : `dict` (optional) | ||
| Dictionary of values and keys to overwrite into the values of | ||
| source_record and store the values in overwrite_dict instead. |
There was a problem hiding this comment.
I don't understand this docstring
There was a problem hiding this comment.
Is this better?
overwrite_dict : `dict` (optional) Mapping specifying the names of columns to overwrite with specified values.
| # explicitly set it here. | ||
| values.append(int(obj_id)) | ||
| field_name = sub_schema.getField().getName() | ||
| overwrite_value = overwrite_dict.get(field_name) |
There was a problem hiding this comment.
suggest instead here:
if field_name in overwrite_dict:
values.append(overwrite_dict[field_name])
| # dateTimeMJD ``seconds``, | ||
| # flux zero point ``counts``, | ||
| # flux zero point error ``counts``] | ||
| values = [visit_info.getExposureId(), |
There was a problem hiding this comment.
I think this would be less fragile to schema changes if you made this a dictionary and then used the keys to specify the columns explicitly in the SQL statement in store_ccd_visit_info above
There was a problem hiding this comment.
I've created a new function much like make_minimal_dia_object_schema and the like for the CcdVisitTable. Let me know if this works for you.
| insert_string = ("?," * len(values[0]))[:-1] | ||
|
|
||
| self._db_cursor.executemany( | ||
| "INSERT OR REPLACE INTO %s VALUES (%s)" % | ||
| (converter.table_name, insert_string), values) | ||
|
|
||
| def get_db_filter_id_from_name(self, filter_name): |
There was a problem hiding this comment.
Maybe go ahead and make the inverse function now (get_db_filter_name_from_id) to save yourself the trouble the future...
| dia_object_record['psFluxMeanErr_%s' % filter_name] = np.nan | ||
| else: | ||
| fluxes = dia_sources.get("psFlux")[ | ||
| dia_sources.get('filterId') == filter_id] |
There was a problem hiding this comment.
It seems gross to have to pass in both filter_id and filter_name here.
Could we load the filterMap table into a (not global, but easily accessible) dictionary at runtime so you can always translate back and forth on the fly?
There was a problem hiding this comment.
I'm not sure of that's any better. The idea here is these functions could easily be ported somewhere else (for instance meas_algorithms) without too much effort. Adding something like a state variable would make it harder to remove later. I could input a pipe.base.Struct here with the filter name and id but that seems a little convoluted to input just two variables.
| @@ -152,27 +190,45 @@ def associate_sources(self, dia_objects, dia_sources): | |||
| return match_result.associated_dia_object_ids | |||
|
|
|||
| @pipeBase.timeMethod | |||
| def update_dia_objects(self, updated_obj_ids): | |||
| def update_dia_objects(self, dia_objects, updated_obj_ids, exposure): | |||
There was a problem hiding this comment.
could just pass in filter_name rather than the whole exposure; you can judge if that's an improvement
tests/test_association_db_sqlite.py
Outdated
| """Test that the filter name mapper to id is working. | ||
| """ | ||
| for filter_name, filter_id in zip(self.assoc_db.config.filter_names, | ||
| [0, 1, 2, 3, 4, 5]): |
There was a problem hiding this comment.
automatically generate the ids from len(filter_names)?
Add filter_id to name test Debug ccdVisit dicts and name to id conversion
morriscb
force-pushed
the
tickets/DM-13685
branch
from
a14151b
to
31de171
Compare
No description provided.