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

Tickets/dm 11479 #1

Merged
merged 1 commit into from Dec 15, 2017
Merged

Tickets/dm 11479 #1

merged 1 commit into from Dec 15, 2017

Conversation

mfisherlevine
Copy link
Collaborator

No description provided.

Copy link
Contributor

@PaulPrice PaulPrice left a comment

Choose a reason for hiding this comment

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

There are several things to think about. In particular, I'm concerned that this isn't useful at all in its current state, and the design is all wrong.

.gitignore Outdated
*.so
*.cfgc
*.pyc
*_wrap.cc
Copy link
Contributor

Choose a reason for hiding this comment

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

Can drop *_wrap.cc --- that's SWIG.

.gitignore Outdated
*.cfgc
*.pyc
*_wrap.cc
*Lib.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Another SWIG-specific extension.

@@ -0,0 +1,22 @@
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is everything preceded by a # as if this were python code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comes straight from the LSST template repo. I think it's to facilitate pasting into python files, but I see no harm in keeping it as-is.

README Outdated
@@ -0,0 +1,11 @@
===============================================================================
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use README.md? GitHub likes that.

@@ -0,0 +1,3 @@
# -*- python -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest dumping this file unless there's anything in the examples directory.

# pair-number is kept, so we have to resolve links and pass in the *original* paths here :(
# Also, there is no "flat-pair" test type, so all FLAT/FLAT imType/testType will appear here
# so we need to filter these for only the pair acquisitions (as the eotest code looks like it
# isn't totally thorough on rejecting the wrong types of data here)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the camera team be persuaded to write the required data in the header?

You should explain what you mean by the "original" filename. I think you mean the filename written by the data acquisition system, which gets changed when we ingest into the data repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to rely on "flat1" and "flat2"? Can't you figure out pairs using the exposure time or something like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The camera team can't change anything, they are in "production mode" and nothing will be changed at this point.

I have improved the explanation of what is meant by "original filename".

You could indeed work it out like that, but that is not how the 3rd party code works, and this code just exists to feed those tasks.

import lsst.cp.pipe as cpPipe

@unittest.skipIf(noEotest, noEotestMsg)
def testClassInstantiation(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

You test for instantiation, but don't actually run anything? I guess that's better than nothing, but not by much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I know, it's just really hard to run anything without a ton of test data, so I spoke to some locals and we thought that at least setting things up was better than nothing.

@@ -0,0 +1,2 @@
@LSST BUILD@
build_lsst @PRODUCT@ @VERSION@ @REPOVERSION@
Copy link
Contributor

Choose a reason for hiding this comment

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

You can dump this file --- it's not used any more.

ups/cp_pipe.cfg Outdated
# table files.
dependencies = {
"required": ["utils", "ndarray"],
"buildRequired": ["boost_test", "pybind11", "ndarray", "pex_exceptions"],
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to revise these lists. Only include those that you use directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry about this, this is another instance of our template package misleading people.

setupRequired(boost >= 1.47.0)
setupRequired(utils >= 4.6.0.0)
setupRequired(pex_exceptions >= 4.6.0.0)
setupRequired(ndarray)
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to revise the list of packages you require. Only include those that you use directly.
Get rid of the >= and everything following.

Add a method to run the eotest analysis using a butler.

This ticket is just to make eotest directly runnable for the TS8 data.
More work will follow to port the algorithms in eotest into the stack,
and make stack-compliant subTasks to do this work inside this task.
@mfisherlevine mfisherlevine merged commit d3025cd into master Dec 15, 2017
@ktlim ktlim deleted the tickets/DM-11479 branch August 25, 2018 05:03
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