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-38957: Ignore new data IDs in QG gen follow-up queries. #327

Merged
merged 2 commits into from May 1, 2023

Conversation

TallJimbo
Copy link
Member

@TallJimbo TallJimbo commented May 1, 2023

These data IDs that were not present in the initial data ID query can come about because we have to drop the post-query region-filtering when doing the follow-up queries for datasets.

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

These data IDs that were not present in the initial data ID query
can come about because we have to drop the post-query region-filtering
when doing the follow-up queries for datasets.
@codecov
Copy link

codecov bot commented May 1, 2023

Codecov Report

Patch coverage: 20.00% and project coverage change: -0.01 ⚠️

Comparison is base (a2643ed) 82.16% compared to head (a2a4987) 82.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #327      +/-   ##
==========================================
- Coverage   82.16%   82.15%   -0.01%     
==========================================
  Files          60       60              
  Lines        6678     6681       +3     
  Branches     1366     1368       +2     
==========================================
+ Hits         5487     5489       +2     
- Misses        917      918       +1     
  Partials      274      274              
Impacted Files Coverage Δ
python/lsst/pipe/base/graphBuilder.py 63.97% <0.00%> (-0.36%) ⬇️
python/lsst/pipe/base/_dataset_handle.py 98.70% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@andy-slac andy-slac 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.

@@ -1097,7 +1097,8 @@ def resolveDatasetRefs(
except MissingDatasetTypeError:
resolvedRefQueryResults = []
for resolvedRef in resolvedRefQueryResults:
assert resolvedRef.dataId in refs
if resolvedRef.dataId not in refs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe catching KeyError in this case would be more efficient, avoiding searching the map twice, but it probably does not matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've seen some examples of Python dict performance on these sorts of questions being completely counterintuitive, so while I do try to avoid double-lookups (usually with if (x := d.get(k)) is not None, these days), that's mostly for consistency rather than any actual expectation of performance.

@TallJimbo TallJimbo merged commit 2bb33ac into main May 1, 2023
8 of 10 checks passed
@TallJimbo TallJimbo deleted the tickets/DM-38957 branch May 1, 2023 14:04
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.

None yet

2 participants