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-32238: Fix unexpected floating point values in drpAssociation task #590

Merged
merged 4 commits into from Oct 28, 2021

Conversation

yalsayyad
Copy link
Contributor

No description provided.

Copy link
Contributor

@morriscb morriscb left a comment

Choose a reason for hiding this comment

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

One quick comment/question.

@@ -178,7 +178,6 @@ def run(self,
catRef.dataId["detector"], nDiaSrc)

if nDiaSrc <= 0:
Copy link
Contributor

@morriscb morriscb Oct 25, 2021

Choose a reason for hiding this comment

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

This should be fine, however, we may still want to be able to handle the case where there are no DiaSources in a given patch. I'm not sure if line 186 will crash if the list of dataframes is empty or not. Also, if it doesn't crash, it still may produce an empty dataframe that causes the typing issues again at a later stage. You could, steal the column names and dtypes from one of the input DiaSource tables and use that to create an empty dataframe with the correct typing.

Copy link
Contributor

@morriscb morriscb Oct 25, 2021

Choose a reason for hiding this comment

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

Yeah, I just confirmed that if the list of DiaSource DataFrames is empty. It exits with ValueError: No objects to concatenate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm. Good point. What would you prefer the behavior be if there are no sources to associate? NoWorkFound? Construct a zero-row table?

Copy link
Contributor

Choose a reason for hiding this comment

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

Up to DRP I would guess. The previous behavior would be to return an empty dataframe and store it. Also, @ebellm made a good point during our meeting today that we should likely write test to catch these cases of no data and one visit with no data to make sure the types hold. We can do that as another ticket or here. Which ever your prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, there was this philosophical debate for what to do with the warps if there were no overlapping calexps, and we added a doWriteEmptyWarp config parameter to enable both. Sounds like you prefer diaAssociationPipeTask to be able to write empty tables, so I added a config parameter here to enable both behaviors. I had to do some work to get SimpleAssociationTask to return a diaObject table with the same schema if it got an empty input dataframe.

You'll find 2 new commits to review:

  1. Handling of situation where there are no diaSources to associate: Option to write out tables with exact same schema when they have no data and when they have data.
  2. I fixed-up the pixelId functor to handle empty inputs. Inspired by convo with @andy-slac on DM-32046 about not backporting the removal of that functor from the diaSource.yaml translation file.

@yalsayyad yalsayyad force-pushed the tickets/DM-32238 branch 3 times, most recently from 5090887 to 7f82232 Compare October 27, 2021 19:01
@yalsayyad yalsayyad force-pushed the tickets/DM-32238 branch 2 times, most recently from e33c51c to fe5f681 Compare October 28, 2021 03:04
to intermediate assocDiaSource DataFrame.
This particular algorithm does not require placeholders.
If the original DataFrame, of which there are no overlapping
diaSources, is empty or malformed to be begin with, it
will changing the concatenated DataFrame's schema.
as when they have data.

- Add config parameter doWriteEmptyTables to control whether
  to write empty tables if there are no overlapping diaSources to
  associate or raise NoWorkFound.
- raise NoWorkFound by default.
@yalsayyad yalsayyad merged commit e49bf47 into master Oct 28, 2021
@yalsayyad yalsayyad deleted the tickets/DM-32238 branch October 28, 2021 06:05
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