Skip to content
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

DM-44387: Add several convenience methods. #106

Merged
merged 4 commits into from
May 23, 2024
Merged

DM-44387: Add several convenience methods. #106

merged 4 commits into from
May 23, 2024

Conversation

ktlim
Copy link
Collaborator

@ktlim ktlim commented May 20, 2024

compute_exposure_id and compute_ccdexposure_id generate unique numeric ids locally without a server round-trip, since summit_utils already depends on afw.

compute_fixed_metadata_namespace is useful for generating query strings.

insert_multiple allows many rows to be inserted at once (e.g. for many detectors in a focal plane).

compute_exposure_id and compute_ccdexposure_id generate unique numeric ids
locally without a server round-trip, since summit_utils already depends on afw.

compute_fixed_metadata_namespace is useful for generating query strings.

insert_multiple allows many rows to be inserted at once (e.g. for many
detectors in a focal plane).
Copy link
Contributor

@mfisherlevine mfisherlevine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, minor comments about potentially moving the static methods. Just make sure to run the pre-commit hooks, guess you forced this to push, right?

@@ -164,6 +164,58 @@ def _handle_post(self, url: str, data: dict[str, Any]) -> requests.Response:
response.raise_for_status()
return response

@staticmethod
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these static methods go to being free-floating functions? I think they'd be useful in other places as well as here, and they're fully generic. Given that, would you be up for putting them in utils.py in here, or some other file. so that they can be imported more naturally than putting them in the consdbClient? (and camelCasing while you're at it, given that?! 😬)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of instrument.lower() is somewhat database-specific (most other places use the official capitalized name or a Python class name with its own distinct capitalization), but yes these could be moved.

def compute_exposure_id(instrument: str, controller: str, day_obs: int, seq_num) -> int:
instrument = instrument.lower()
if instrument == "latiss":
from lsst.obs.lsst.translators import LatissTranslator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these imports particularly costly or something? I think everything in summit_utils has generally already imported the world, so if that's why these imports aren't at the top then I don't think it really matters. Was that the reason for putting them here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the only things in ConsDbClient that required anything stack-specific. Moving them out would mean that there's no need to hide them.

@ktlim ktlim force-pushed the tickets/DM-44387 branch 3 times, most recently from 6e626bc to b0539d6 Compare May 23, 2024 18:02
@ktlim ktlim merged commit eb62f08 into main May 23, 2024
4 checks passed
@ktlim ktlim deleted the tickets/DM-44387 branch May 23, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants