-
Notifications
You must be signed in to change notification settings - Fork 0
fix_sso_w_sourcegen #28
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,19 +75,19 @@ def ingest_jpl_parquet_file( | |
| sso = client.sso.get_sso_MPC_id(MPC_id=mpc_id)[0] | ||
|
|
||
| for _, row in tqdm( | ||
| rows[::10].iterrows(), desc=f"Ingesting {designation}", total=len(rows[:10]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, I must have left that in |
||
| rows.iterrows(), desc=f"Ingesting {designation}", total=len(rows) | ||
| ): | ||
| flux = None | ||
| flux = 200 * u.mJy # Default flux if not provided | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Who came up with this and why? Probably should be configurable.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. me - and yes itshoudl be but a flux of None causes it to fail so I had to give it some finite value
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or - alternatively we have a 'monitored' flag that allows us to do forced photometry on sources that we don't have fluxes for (i.e. asteroids or transients / dave's favorite sources)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uh what error does it raise? |
||
| if "flux_mJy" in row.index and pd.notna(row["flux_mJy"]): | ||
| flux = row["flux_mJy"] * u.mJy | ||
|
|
||
| unix_time = Time(row["julian_day"], format="jd").unix | ||
| unix_time = Time(row["julian_day"], format="jd") | ||
|
|
||
| client.ephem.create_ephem( | ||
| sso_id=sso.sso_id, | ||
| MPC_id=mpc_id, | ||
| name=name, | ||
| time=int(unix_time), | ||
| time=unix_time, | ||
| position=ICRS( | ||
| ra=float(row["ra_deg"]) * u.deg, | ||
| dec=float(row["dec_deg"]) * u.deg, | ||
|
|
||
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.
The SO in SOCAT is actually for 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.
create_ephem doesn't raise an error but somethign else did further down the line. It might have just been get_forced_photometry_sources or something. which I could have skirted by doing flux selection later
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.
Can you write a test that fails or otherwise give me something I can use to reproduce?
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.
ok, just added test -- the cause is in SourceGenerator
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.
Ah ok well I'll run this but almost certainly the problem is that flux is one of the interpolated values in
SourceGenerator. It was asked that I add this feature to support the ability to filter sources by flux. This is then sort of a high level question of what we want to do with sources without flux estimates. We can simply returnNonefor the flux value if the source insocathas no flux estimate [i.e., return (x,y, None)], or we can change the shape of the return type to be only(x,y)as opposed to(x,y,flux.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.
I think we want to just return None for the flux; i.e. .at_time() would return (postition,None) and we don't have to worry about interpolating it.
We will have to decide what that means for forced photometry.
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.
Addressed in PR #29