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-15514: Write insertFakeObjectsTask to add fake sources for QA purposes #140

Merged
merged 1 commit into from Jun 6, 2019

Conversation

sr525
Copy link
Contributor

@sr525 sr525 commented Feb 21, 2019

No description provided.

@natelust
Copy link
Contributor

The commit message conveys almost no information, try a different title and or include more information in the commit message body

@@ -39,6 +39,8 @@ class ForcedPhotCoaddConfig(ForcedPhotImageConfig):
optional=True
)

fakesAdded = lsst.pex.config.Field(dtype=bool, default=False, doc="Have fakes been added?")
Copy link
Contributor

Choose a reason for hiding this comment

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

This Field should follow the structure of the other fields already in the file, aka multi-line

Documentation should be statements and not questions, perhapse something like "Fields indicates if Fake objects are present in input dataset"

Consider a different name for the field variable, like fakesPresent of fakesInserted as it is closer to the use of this variable in English terms, but this is just a suggestion from someone reading it, there is nothing wrong with how it is

name = self.config.coaddName + "Coadd_calexp"

if self.config.fakesAdded:
name = "fakes_" + self.config.coaddName + "Coadd_calexp"
Copy link
Contributor

Choose a reason for hiding this comment

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

In general the python community seems to be going toward using the format method or f strings instead of the addition operator, so it might be useful to get in the habit. This code would then be

name = f"fakes_{self.config.coaddName}Coadd_calex"

What you have will run fine, just using the opportunity to maybe pass along some tips. There is a runtime performance you gain by using f strings or format vs concatenation, but in this case the micro optimization does not really matter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it preferable to use this way or to have a consistent formatting throughout the file?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://developer.lsst.io/coding/intro.html?highlight=consistency#deviating-from-the-dm-style-guides (second bullet):

Coding consistency is very important but sometimes the style guide doesn’t apply either due to lack of a definitive rule or the circumstances of the specific code segment logically dictate otherwise. When in doubt, use your best judgment or ask the lead developer.

Here are two plausible reasons to break a particular rule:

  • When applying the rule would make the code less readable, even for someone who is used to reading code that follows the rules.
  • To be consistent with surrounding code that also breaks it (maybe for historic reasons)—although this is also an opportunity to clean up someone else’s mess.

@sr525 sr525 merged commit d4562c7 into master Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants