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-29271: PTC task: Refactoring/simplification of code ported from Pierre Astier's repository. #76

Merged
merged 11 commits into from Mar 31, 2021

Conversation

plazas
Copy link
Contributor

@plazas plazas commented Mar 23, 2021

No description provided.

@plazas plazas requested a review from czwa March 23, 2021 20:42
Copy link
Contributor

@czwa czwa left a comment

Choose a reason for hiding this comment

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

I think I have too many questions to approve directly. This definitely helps squash some of the unused code/odd single-call functions, but I'm still a bit confused on whether more could be done.

python/lsst/cp/pipe/ptc/cpExtractPtcTask.py Outdated Show resolved Hide resolved

return mu, varDiff, covDiffAstier

def fillPtcDatasetAmp(self, ptcDataset, ampName, rawExpTime=[np.nan], rawMean=[np.nan], rawVar=[np.nan],
Copy link
Contributor

Choose a reason for hiding this comment

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

This method makes me think there maybe should be a method (or set of methods) on the PTC object to assist in construction. This sets everything to single-element lists for each amplifier property, correct? I think my idea is an initAmp method that accepts lists like this is doing, but that then checks that all the lengths for supplied values match. This could then be used here as well as in CpSolvePtcTask when the dataset is filled with the collated results, and also even in the PtcDataset.__init__. Maybe on the next pass at refactoring.

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 placed the amp-filling function as part of the ptcDataset code inip_isr.

python/lsst/cp/pipe/ptc/cpExtractPtcTask.py Outdated Show resolved Hide resolved


def parseData(dataset, params):
def parseData(dataset):
Copy link
Contributor

Choose a reason for hiding this comment

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

This function just creates the CovFit objects that are later fit, correct? I'm unhappy with it returning a dict but calling the variable name a list, but that's a side point. I'm not entirely sure I see the point of generating a dictionary of CovFit objects, then iterating over the items in that dict instead of just doing a normal loop over amplifiers stored in the dataset, instantiating a CovFit object, running the fit, storing the useful fit values in the dataset. Are the CovFit objects used downstream in a way that I'm just not thinking about correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the dictionary with the CovFit objects is then used in cpSolvePtcTask by getOutputPtcDataCovAstier when getting the output dataset.

Copy link
Contributor Author

@plazas plazas Mar 29, 2021

Choose a reason for hiding this comment

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

i'll change the suffixes from List to Dict.

Copy link
Contributor

@czwa czwa left a comment

Choose a reason for hiding this comment

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

I think this answers all of my concerns, thanks!

@plazas plazas merged commit f4ce030 into master Mar 31, 2021
@plazas plazas deleted the tickets/DM-29271 branch March 31, 2021 20:22
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