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-27209: Update for new quantum graph API #7

Merged
merged 2 commits into from Oct 19, 2020
Merged

Conversation

timj
Copy link
Member

@timj timj commented Oct 16, 2020

No description provided.

@timj timj requested a review from natelust October 16, 2020 22:52
Copy link
Contributor

@natelust natelust left a comment

Choose a reason for hiding this comment

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

Some suggestions on how to do these operations more efficiently. What you wrote seems like it will work, so take whatever info you think is best.

for quantum_node in graph:
for key, value in quantum_node.quantum.inputs.items():
for dataset in value:
if dataset.datasetType.name not in dataset_types_to_exclude:
Copy link
Contributor

Choose a reason for hiding this comment

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

quantum.inputs is already a mapping of DatasetType. As it is now, key is not used. If this was rearranged to:

...
for key, value in quantum_node.quantum.inputs.items():
    if key.name not in dataset_types_to_exclude:
        found = {butler.registiry.queryDatasets(key.name, collections=..., dataId=dataset.dataId) for dataId in value}
...

It would save a for loop over a lot of elements that will be excluded by dataset_types_to_exclude

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 have rejigged a bit based on the suggestion above but I can't use that line that does the loop over refs in queryDatasets because queryDatasets returns an interator and not DatasetRef and so you end up with a set of iterators. It's easier to use still use an explicit loop over refs (but still inside the if-check).

One thing that does confuse me is that the quantum_node.quantum.inputs is returning a proper DatasetRef with the id attached so the reality is that this queryDataset call is returning the same thing it had to start with. Not entirely sure that we expect that or not so maybe it's a special case for this particular example.

for key, value in quantum_node.quantum.inputs.items():
for dataset in value:
if dataset.datasetType.name in ("raw",):
items.append(dataset)
Copy link
Contributor

Choose a reason for hiding this comment

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

This would better be written as:

for quantum_node in graph.findQuantaWithDSType("raw"):
    if "raw" in quantum_node.quantum.inputs:  # Verify that raw is an input and not an output
        items.extend(quantum_node.quantum.inputs["raw"])

This should cut down on the number of iterations as findQuantaWithDSType uses precomputed dictionaries.

@timj timj merged commit 708f91c into master Oct 19, 2020
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