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.

Sign up for GitHub

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

Jump to bottom

DM-23195: DM-21221 broke cp_pipe due to lack of tests #26

Merged
merged 5 commits into from Feb 28, 2020
Merged

DM-23195: DM-21221 broke cp_pipe due to lack of tests #26

merged 5 commits into from Feb 28, 2020

Conversation

plazas
Copy link
Contributor

@plazas plazas commented Feb 21, 2020

No description provided.

jdswinbank
jdswinbank requested changes Feb 27, 2020
View reviewed changes
python/lsst/cp/pipe/ptc.py Outdated

Returns
-------
dataset : `lsst.cp.pipe.ptc.PhotonTransferCurveDataset`
Copy link
Contributor

Choose a reason for hiding this comment

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

This dataset is the same object as the dataset parameter, but has been modified (right?). Let's document that — rather than just repeating the same words, let's be explicit about what this method changes in that object.

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 expanded the docstring with clarifications.

@jdswinbank
Copy link
Contributor

There are actually three separate changes here, and I think it would be helpful to split them into three separate commits:

  • Change from visit to expId. The commit message for this should explain why it was necessary (e.g. by referring to the ticket which changed definitions elsewhere).
  • Correct the method call from fitPtcAndNl to fitPtcAndNonLinearity.
  • Adjust the definition of fitPtcAndNonLinearity to return dataset.

@jdswinbank
Copy link
Contributor

Also, remember the character limit on the first line of commit messages: see our guidelines.

jdswinbank
jdswinbank reviewed Feb 27, 2020
View reviewed changes
python/lsst/cp/pipe/ptc.py
@@ -354,7 +354,7 @@ def runDataRef(self, dataRef, visitPairs):
lookupTableArray = np.zeros((numberAmps, numberAduValues), dtype=np.float32)

# Fit PTC and (non)linearity of signal vs time curve, produce linearizer
self.fitPtcAndNonLinearity(dataset, lookupTableArray, ptcFitType=self.config.ptcFitType)
dataset = self.fitPtcAndNonLinearity(dataset, lookupTableArray, ptcFitType=self.config.ptcFitType)
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be helpful to clarify the comment before this line. Given the comment “Fit..., produce linearizer”, I would expect this to return something called linearizer, not dataset.

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 added a couple of extra comments in there.

@plazas
Copy link
Contributor Author

plazas commented Feb 28, 2020

Thank you for your comments. I have addressed them.

@plazas plazas merged commit 079e426 into master Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdswinbank jdswinbank jdswinbank requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@plazas @jdswinbank