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-16127: Move fgcmcal from lsst-dm to lsst #2

Merged
merged 1 commit into from Nov 20, 2018
Merged

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Oct 23, 2018

No description provided.

@parejkoj
Copy link
Contributor

parejkoj commented Nov 2, 2018

Another thing: you need to add setupRequired(fgcm) to the table file, since I think it's a dependency of this package.

@parejkoj
Copy link
Contributor

parejkoj commented Nov 5, 2018

A useful addition to the cookbook would be a comment on roughly how long each step will take to complete on lsst-dev.

@erykoff
Copy link
Contributor Author

erykoff commented Nov 5, 2018

Timing information has now been added to the cookbook.

Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

You need license info in your python files. Unless you're going with a different license, which is certainly possible, this is the LSST boilerplate: https://developer.lsst.io/stack/license-and-copyright.html?highlight=license#python-preamble

Question about convergence criteria: you say "15 for testing, 50+ for a full run": is there a better way to understand whether one has chosen an appropriate number of steps? Can you improve the step size or convergence criteria in the code to make that unnecessary?

README.md Outdated

This is LSST stack code to interface with the FGCM package. See
http://adsabs.harvard.edu/abs/2018AJ....155...41B for the paper describing the
method, and https://github.com/erykoff/fgcm for a Python implementation that is
method, and https://github.com/erykoff/fgcm (as forked in
https://github.com/lsst/fgcm ) for the Python implementation that is
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth mentioning here that the lsst fork follows the lsst-dev branch instead of master?

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 it is.

@@ -1,5 +1,8 @@
#!/usr/bin/env python
# See COPYRIGHT file at the top of the source tree.
import matplotlib
matplotlib.use("Agg") # noqa
Copy link
Contributor

Choose a reason for hiding this comment

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

These can be marked #noqa E402 to make it more explicit.

@parejkoj
Copy link
Contributor

parejkoj commented Nov 8, 2018

Since one of the goals here was to give fgcmcal a proper review before it joins lsst_distrib, I've forked (so I don't accidentally muck anything up) and made a separate fake PR of the end of this branch vs. the initial commit, to make commenting easier:

parejkoj#1

As you push to this branch, I'll sync that branch up when you tell me it's appropriate.

@r-owen r-owen force-pushed the tickets/DM-16127 branch 5 times, most recently from 46c0798 to d68ff46 Compare November 12, 2018 19:48
@parejkoj
Copy link
Contributor

Question from Eli on the detailed review that @TallJimbo or @r-owen might know the answer to: parejkoj#1 (comment)

Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

Thank you for dealing with my review comments. I know it was a lot, but the code documentation is definitely better, and I think the logical flow of the code is clearer.

Add fgcm, testdata_joincal to required and optional packages; Add setup.cfg for
flake8 settings and exclusions; Move matplotlib backend setting; Update
installation instructions; Use
LoadIndexedReferenceObjectsTask.makeMinimalSchema to make reference catalog
schema; Add approximate timing information to cookbook; Update copyright and
licenses; Rename variables as needed (e.g. mag -> instMag); Remove unused code;
Streamline methods; expand documentation; add missing docstrings; add
references to tickets for future updates; Use cameraGain from butler; add
.travis.yml; add module docstrings; Better handle on-disk angle units; Update
README.
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