-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fix aperture estimation, use local bkg from an annulus for aperture photometry, add the web interface to explore the end results #16
Conversation
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 did a quick review and left some minor comments. Overall is not easy to review the changes without tests. The last test was running forever so I could not fully check it. Anyway, go ahead with the merge
|
||
|
||
def get_target_fwhm_aperpix(redfL): | ||
import numpy as np |
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.
already imported at the beginning of the file
iop4lib/utils/__init__.py
Outdated
|
||
|
||
|
||
|
||
|
||
|
||
|
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.
remove all the extra blank lines (or better run black formatter)
every staff member should be able to edit the catalog, all others models should not be manually edited except by the iop4 pipeline
permissions dont make sense in the debug server, and in production they are better set using users and groups
This versions passes all tests in both Ubuntu and macOS, and is working for the VHEGA site. I merge if you don't oppose. Sorry for the Monster PR... but it is growing only bigger. Applying the black or any other formatter will destroy git history. I'm looking what's the best way to deal with it, any idea? I think it can be postponed anyway. |
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.
Go ahead with the merging. Formatting is another matter
This PR: