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

Add ability to add fake sources selectively to either the template or… #398

Merged
merged 1 commit into from Jul 23, 2020

Conversation

sr525
Copy link
Contributor

@sr525 sr525 commented Jul 14, 2020

… the calexp.

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.

Looks like a good start though some of the text and logic is confusing. Made some suggestions on the naming as requested.

What is the plan for MkTransientFakesCatalogTask? It looks to be very similar to processCcdWithFakes.

@@ -190,6 +190,14 @@ class InsertFakesConfig(PipelineTaskConfig,
default="deep",
)

template = pexConfig.Field(
doc="If the image is to be used as a template then set to True. This will add only sources with"
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc description is a bit confusing and could use some simplifying. Especially the third sentence. As for a suggestion for the names "template" seems fine, but perhaps "image" could be changed to "visistExposure" or "ccdVisitExposure"?

Copy link
Contributor

Choose a reason for hiding this comment

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

The columns themselves might need a bit more descriptive name like isTemplateSource or insertIntoTemplate. Might help with naming to think of them like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case anyone wants to put sources with different fluxes into different coadds, I changed the column names to 'isTemplateSource' and 'isNotOnlyTemplateSource'.

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 have tried to make this doc clearer and will add more details to the community post instructions when I update them. (Which I will do once we have finalised the discussion on this.)

-----
If the cofig option 'template' is set to True then only objects with the 'template' column
in the input fakes catalog will be added to the given image. If the 'image' column is set
to True then objects will only be added if the config option 'template' is set to False. If
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, the notes here is a bit confusing and could use some edits.

@@ -555,6 +570,22 @@ def cleanCat(self, fakeCat):
self.log.info("Removing %d rows with nBulge or nDisk outside of %0.2f <= n <= %0.2f" %
(badRows, minN, maxN))

if self.config.template:
try:
goodRows = (fakeCat["template"])
Copy link
Contributor

Choose a reason for hiding this comment

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

goodRows and badRows is a bit unclear what you're actually doing here. I assume this is left over from another test but the rows for these settings aren't bad or good. They are just used or aren't.

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 changed the names throughout this function as the point you make here is valid not just for this bit and I would like to keep the naming consistent.



class MkTransientFakesCatalogTask(PipelineTask, CmdLineTask):
"""Insert fake objects into calexps.
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 doc copied from a previous task? Doesn't line up with the task name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mkTransientFakesCatalogTask was supposed to be deleted, I'm not sure why it wasn't deleted remotely.

super().setDefaults()


class MkTransientFakesCatalogTask(PipelineTask, CmdLineTask):
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 task name communicating what the task is doing? Looks to insert into an image and run calibrate to detect sources. The name currently reads like it's creating a catalog of fakes.

@sr525 sr525 force-pushed the tickets/DM-25662 branch 4 times, most recently from a41b110 to facb0c9 Compare July 21, 2020 20:25
@sr525 sr525 merged commit 2a9f694 into master Jul 23, 2020
@timj timj deleted the tickets/DM-25662 branch February 18, 2021 15:49
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