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
jointcal running on test data (DM-5297) #12
Conversation
0fc5412
to
80a4f43
Compare
@@ -10,7 +10,7 @@ | |||
import eups | |||
|
|||
from lsst.afw import geom, coord, table | |||
from lsst.meas.astrom import astrometry | |||
from lsst.meas import astrom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest from lsst.meas.astrometry import LoadAstrometryNetObjectsTask
I worry that it's hard for a user to tell what is brought in when you import all of lsst.meas (it's not a usage I've ever seen in our code before). Note that you probably don't need the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. (though "from lsst.meas import astrom" shouldn't pollute the namespace at all.)
Can you modernize the tests to stop using suite whilst you are in there. |
ipdb.set_trace() | ||
|
||
|
||
def suite(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix to remove suite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Judging from SQR-012, the correct thing to do replace that whole block with this, right?
if __name__ == "__main__":
lsst.utils.tests.init()
unittest.main()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. And then see if it works. Technically add the setup_module function as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, did that too. Seems to work.
Ok, I've cleaned up everything described above. I fear that rebase and squashing will be a fair bit of effort: should I bother? |
I suggest having a try at rebasing. Make a copy of your repo before you start, so you can dig out if you make a mess. |
e328668
to
3aaa964
Compare
3aaa964
to
6dab09e
Compare
Replace boost::shared_ptr with std::shared_ptr Replace boost::dynamic_pointer_cast to std::dynamic_pointer_cast deboostify SWIG Note that there still is the boost::intrusive_ptr to deal with.
This will be dealt with in a proper manner as part of DM-5501.
Cleanup print statements and size checks in jointcalTask. Compute rms of 2 and 10 catalog runs as my test metric. It's not perfect, but at least it provides a way to tell if I've broken anything. Still keeping around the plotting code, as it's very useful for diagnostics. Add validation_data_jointcal as optional dependency for the test calexps.
6dab09e
to
86f7b08
Compare
Review complete, code builds, tests run. |
Jointcal now runs on some lsstSim data (twinkles 1), and includes a pseudo-validation test, to check that the astrometric fits improve as we add more images. There are several hacks to get around the metadata problems that I will deal with in a separate ticket.