-
Notifications
You must be signed in to change notification settings - Fork 20
DM-53903: Update sso association for new MPC columns #1236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
isullivan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase to remove the merge commit from the history.
| # All the below designation-related code are copied from B612's adam_core | ||
| # adam_core should eventually be added as an external dependency, and this | ||
| # should be replaced with imports. | ||
| # should be replaced with imports. DM-59307 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this DM-53907?
| assocSourceMask = diaSourceCatalog["ssObjectId"] != 0 | ||
| unAssocObjectMask = np.logical_not(maskedObjects['associated'].value) | ||
| ssSourceData = np.array(ssSourceData) | ||
| dtypes = [type(d) if d is not np.ma.core.MaskedConstant else float |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be type(d) if type(d) is not np.ma.core.MaskedConstant...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how I typed it, but linter demanded that I change it to this.
| ssSourceData = Table(ssSourceData, names=colnames, dtype=[str] + [np.float64] * (len(colnames) - 1)) | ||
| ssSourceData = Table(ssSourceData, names=colnames, dtype=dtypes) | ||
| if 'MPCORB_created_at' in ssSourceData.columns: | ||
| for c in ['MPCORB_created_at', 'MPCORB_updated_at', 'MPCORB_fitting_datetime']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code is a little confusing, and could use a code comment. If this first for loop is just over-writing any values of 'MPCORB_created_at' etc.., then I think it might be more clear to just write each explicitly on their own line. For example: ssSourceData['MPCORB_created_at'] = 0. The next loop setting these columns to int64 is fine as-is.
| return vectors | ||
|
|
||
| def _return_empty(self, diaSourceCatalog, emptySolarSystemObjects, source_column): | ||
| def _return_empty(self, diaSourceCatalog, emptySolarSystemObjects, source_column): # TODO: UPDATE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this TODO comment mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really encompassed by the next todo, so to avoid dueling TODOs I'll remove this one.
403d2b4 to
87e3905
Compare
87e3905 to
c688bd8
Compare
No description provided.