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-18739: Generate defects from standardized format #66

Merged
merged 15 commits into from Jun 21, 2019

Conversation

SimonKrughoff
Copy link
Contributor

No description provided.

@SimonKrughoff SimonKrughoff requested a review from r-owen May 30, 2019 16:54
@SimonKrughoff
Copy link
Contributor Author

Note that defects in standard form now reside in obs_test_data.

Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

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

One bug to fix and an associated test to write. Otherwise it looks great.

data/ReadMe.md Outdated Show resolved Hide resolved
data/utils/defectsFromBias.py Outdated Show resolved Hide resolved
data/utils/defectsFromBias.py Outdated Show resolved Hide resolved
data/utils/defectsFromBias.py Show resolved Hide resolved
assert len(bboxList) == len(test2BBoxList)
for boxA, boxB in zip(bboxList, test2BBoxList):
assert boxA == boxB
test2defectList = Defects.readText(DefectsPath+".ecsv")
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it work to include the ".ecsv" suffix on DefectsPath itself -- would the correct file get written? If so, I think it would make the code a bit simpler and easier to understand. Also the description string would give the correct path for the output file (right now it is missing the ".ecsv" suffix).

Copy link
Member

Choose a reason for hiding this comment

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

I think you want to get the return value from writeText and use that directly in readText (writeText forces the file extension based on the type of text file it is writing). The debug print message should also use the actual name used as returned by writeText.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds reasonable, though it means that the full name of the file cannot be included in the help string for the command line parser is produced. I suppose one could always print a message when saving the data.

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 believe I have addressed this.

data/utils/defectsFromBias.py Outdated Show resolved Hide resolved
data/utils/defectsFromBias.py Outdated Show resolved Hide resolved
This means the tickets/DM-18739 ticket will need to be merged with the
tickets/DM-19857 branch of ap_verify
@@ -0,0 +1,14 @@
# Config override for lsst.pipe.tasks.IngestCalibsTask
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean IngestDefectsTask?

@SimonKrughoff SimonKrughoff merged commit 1339d21 into master Jun 21, 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants