-
Notifications
You must be signed in to change notification settings - Fork 20
DM-53310: New generation of SS* tables #1194
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
8ea6115 to
ddc7623
Compare
242efb0 to
e3cf8e3
Compare
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 move package imports to the top of files where possible.
- A few workarounds need tickets filed and referenced at the relevant location in the code.
- Check the formatting of the new
mpcorbconnectiondimensions.
| doc="Minor Planet Center orbit table used for association", | ||
| name="mpcorb", | ||
| storageClass="DataFrame", | ||
| dimensions={}, |
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.
| dimensions={}, | |
| dimensions=(), |
| f"Done. {len(ssSourceTable)} observations, {np.unique(ssSourceTable['ssObjectId']).size} objects." | ||
| ) | ||
|
|
||
| # Compatibility for pre RFC-1138 ss_source_associated tables |
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 file a ticket to remove this compatibility in the future, and add a reference to that ticket here.
| meta_kernel_file.write(meta_kernel_text) | ||
| self.log.info('Sorcha process begun') | ||
|
|
||
| # FIXME: this is a workaround for breakage in sbpy 0.5.0 caused |
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 file a ticket to remove the workaround, and reference the new ticket here.
| if not os.path.exists(eph_path): | ||
| from time import sleep | ||
| print('not os.path.exists()', eph_path) | ||
| sleep(1000) |
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 is the sleep(1000) necessary?
| ephOffsetRa = np.array(residual_ras * np.cos(np.radians(ssSourceData["dec"]))) * 3600 | ||
| ephOffsetDec = np.array(residual_decs) * 3600 |
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 add a comment explaining why these are multiplied by 3600
| if "discoverySubmissionDate" in row.dtype.names: # DP2 does not have this field | ||
| # FIXME: here I arbitrarily guess we discover everything 7 days | ||
| # after first obsv. we should really pull this out of the obs_sbn tbl. | ||
| row["discoverySubmissionDate"] = row["firstObservationMjdTai"] + 7.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.
This seems guaranteed to be wrong. Will that be a problem?
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 see now that discoverySubmissionDate is excluded from the final schema, so I guess it is fine for now.
| from astropy.coordinates import get_body_barycentric_posvel, solar_system_ephemeris | ||
| import astropy.units as u |
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.
Move these imports to the top of the file
| from astropy.coordinates import SkyCoord, HeliocentricEclipticIAU76 | ||
| import astropy.units as u |
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.
Move imports to the top of the file and deduplicate
| ], | ||
| ).reset_index(drop=True) | ||
|
|
||
| from functools import partial |
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.
Move import
python/lsst/pipe/tasks/ssp/util.py
Outdated
| import pyarrow as pa | ||
| import pyarrow.compute as pc |
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.
Move imports
e3cf8e3 to
671028f
Compare
671028f to
ac24ecf
Compare
ac24ecf to
43a9c05
Compare
Update the generation code for Solar System-related tables, implementing https://ls.st/RFC-1138.