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-31217: Add ConsolidateHealSparsePropertyMapsTask #577

Merged
merged 3 commits into from Sep 29, 2021
Merged

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Sep 24, 2021

No description provided.

Copy link
Contributor

@taranu taranu left a comment

Choose a reason for hiding this comment

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

Minor clarifications requested.

def __init__(self, *, config=None):
super().__init__(config=config)

# Remove connections from those that are not configured
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would there be unconfigured connections? Is this something a user should be warned about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've improved the comments here and when I used the same pattern for the tract-based maps. But the idea is that we have to create all possible connections that are in the registry, and then remove those that weren't configured to run to leave the subset of connections that we want to use.



class ConsolidateHealSparsePropertyMapTask(pipeBase.PipelineTask):
"""Task to consolidate HealSparse property maps."""
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be helpful to document somewhere what exactly is being consolidated as it's not obvious to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am expanding this, and adding rst task documentation which I had forgotten to add.

# Create input/output connections for all possible maps defined in the
# registry.
for name in BasePropertyMap.registry:
vars()[f"{name}_map_min"] = pipeBase.connectionTypes.Input(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the preferred pattern for batch assignment of connections? It seems sensible on second thought but was unexpected to me at first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only way that I know of to programatically make connections which must be class instance variables. I've added additional comments, including a link to the magic stack overflow which told me how to do this: https://stackoverflow.com/questions/2519807/setting-a-class-attribute-with-a-given-name-in-python-while-defining-the-class

@erykoff erykoff merged commit fc06ea4 into master Sep 29, 2021
@erykoff erykoff deleted the tickets/DM-31217 branch September 29, 2021 03:44
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