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-31033: Move DiaCalculation from a subtask of AssociationTask to a subtask of DiaPipe. #120

Merged
merged 3 commits into from Jul 26, 2021

Conversation

morriscb
Copy link
Contributor

No description provided.

Remove DiaSource duplicate test.

Fix DiaObject dup test.

Add function to test for duplicates.

Add function for DataFrame index checking.

This is to allow for the usage of Mocks in unittesting.

Fix typo.
Copy link
Contributor

@kherner kherner left a comment

Choose a reason for hiding this comment

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

Generally looks good; see my one comment about killing the entire init function in association.py as opposed to just the makesubtask part.

@@ -89,10 +59,6 @@ class AssociationTask(pipeBase.Task):
ConfigClass = AssociationConfig
_DefaultName = "association"

def __init__(self, **kwargs):
pipeBase.Task.__init__(self, **kwargs)
self.makeSubtask("diaCalculation")
Copy link
Contributor

Choose a reason for hiding this comment

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

Deleting the entire init function won't break anything else, will it? Certainly deleting the makeSubtask call makes sense, but you're killing the entire init function.

Copy link
Member

Choose a reason for hiding this comment

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

The first line is calling the parent class init (written before super() was common) so if you are removing the second line the first line is not doing anything on its own.

@morriscb morriscb merged commit 4f3984a into master Jul 26, 2021
@morriscb morriscb deleted the tickets/DM-31033 branch July 26, 2021 19:17
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

3 participants