-
Notifications
You must be signed in to change notification settings - Fork 33
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
Save ZTF alerts as sources by objectId #44
Save ZTF alerts as sources by objectId #44
Conversation
w = df["candid"] == str(candid) | ||
|
||
if candid is None or sum(w) == 0: | ||
candids = {int(can) for can in df["candid"] if not pd.isnull(can)} |
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.
Got sick of pandas relationship with long ints and implemented it this way.
log(f"Failed to post photometry of {objectId}") | ||
# do not return anything yet | ||
self.clear() | ||
|
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.
Monsieur knows a lot about perversion, don't you think @dannygoldstein? :) Hope you appreciate the "beauty"!
except Exception: | ||
log(f"Failed to post photometry of {objectId}") | ||
# do not return anything yet | ||
self.clear() |
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.
If you don't do this, the photometry handler would try to .write()
merge conflicts |
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.
Main comment is: what happens when a user attempts to save an alert to a group without permissions to see the data or that the object exists?
E.g., an object with all PID=3 alerts saved to the sitewide_group
import * as sourcetActions from "../ducks/source"; | ||
import FormValidationError from "./FormValidationError"; | ||
|
||
const SaveAlertButton = ({ alert, userGroups }) => { |
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 happens when a user tries to save an object with only PID=3 alerts to the sitewide group?
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.
Great point, addressed here. It will now fail.
@@ -141,10 +207,313 @@ def get(self, objectId: str = None): | |||
_err = traceback.format_exc() | |||
return self.error(f'failure: {_err}') | |||
|
|||
@auth_or_token | |||
async def post(self, objectId): |
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.
why async?
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.
because I am calling K asynchronously and awaiting the results; you can await
only inside an async
function.
if len(alert_data) > 0: | ||
alert_data = alert_data[0] | ||
else: | ||
return self.error(f"{objectId} not found on Kowalski") |
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.
As above, what about the case where someone saves an object to a group without permissions to see the data?
ztf_filters = {1: 'ztfg', 2: 'ztfr', 3: 'ztfi'} | ||
df['ztf_filter'] = df['fid'].apply(lambda x: ztf_filters[x]) | ||
df['magsys'] = "ab" | ||
df['zp'] = 25.0 |
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 used downstream?
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.
No, removed.
@dannygoldstein Feedback addressed, please re-review. |
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.
Some minor comments but otherwise looks good
group_ids = [ | ||
int(_id) | ||
for _id in group_ids | ||
if int(_id) in user_accessible_group_ids |
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.
If I understand this correctly, it seems like we just silently ignore any groups the user doesn't have access to. Should we send a specific error if a group_id passed in isn't part of the user_accessilbe_group_ids? To let someone know if they think they're part of a group but aren't for whatever reason?
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.
Added a check on that
import { useForm, Controller } from "react-hook-form"; | ||
|
||
import * as alertActions from "../ducks/alert"; | ||
import * as sourcetActions from "../ducks/source"; |
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.
import * as sourcetActions from "../ducks/source"; | |
import * as sourceActions from "../ducks/source"; |
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.
Fixed
} else { | ||
setDialogOpen(false); | ||
reset(); | ||
await dispatch(sourcetActions.fetchSource(alert.id)); |
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.
await dispatch(sourcetActions.fetchSource(alert.id)); | |
await dispatch(sourceActions.fetchSource(alert.id)); |
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.
Fixed
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.
LGTM
In this PR:
extensions/skyportal/skyportal/handlers/api/alert.py
:ZTFAlertHandler.post
to save ZTF alerts as sources by theirobjectId
, e.g.api('POST', '<fritz_instance_location>/api/alerts/ztf/<objectId>'), {"candid": <candid>, "group_ids": <list of group ids>}
. Under the hood, F pulls the required data from K, massages them, and posts to SP's/api/sources
,/api/photometry
, and/api/thumbnail
. Ifcandid
is not provided, metadata and thumbnails will be pulled from the latest alert/candid. If no list of group_ids is provided, the object will be saved to all user's groupstornado.httpclient
instead ofrequests
extensions/skyportal/static/js/components/ZTFAlert.jsx
extensions/skyportal/static/js/components/SaveAlertButton.jsx
SaveCandidateButton.jsx
with a couple modifications necessary hereextensions/skyportal/static/js/components/Filter.jsx