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

Add verbose mode to ellipse fitting and modeling #947

Closed
wants to merge 7 commits into from
Closed

Add verbose mode to ellipse fitting and modeling #947

wants to merge 7 commits into from

Conversation

ibusko
Copy link
Contributor

@ibusko ibusko commented Aug 15, 2019

This addresses #891 and #823

@astropy-bot astropy-bot bot added the isophote label Aug 15, 2019
@pep8speaks
Copy link

pep8speaks commented Aug 15, 2019

Hello @ibusko! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-08-15 21:31:12 UTC

@bsipocz bsipocz added this to the 0.8 milestone Aug 15, 2019
@bsipocz
Copy link
Member

bsipocz commented Aug 15, 2019

What about having logger verbosity rather than explicit prints? That would fit much nicer into a pipeline environment.

@ibusko
Copy link
Contributor Author

ibusko commented Aug 15, 2019

@bsipocz that would be nice too, but I wonder how that would work in a, say, notebook environment. I never though of this package as something that can be used in a pipeline environment, due to the highly interactive nature of the data analysis that it performs. The purpose of the direct prints at stdout is to provide the user with instant visual feedback on the progress, as would be with a, say, progress bar.

But if we can have the logger perform by default as the plain stdout prints we have now, it would be easy to convert the print code to logger calls.

@larrybradley
Copy link
Member

@ibusko Astropy's logger works fine in a notebook environment. The advantage of the logger, of course, is it can be turned off by the user.

from astropy import log
log.info('my message')

@ibusko
Copy link
Contributor Author

ibusko commented Aug 16, 2019

@larrybradley that's good. Still, I wonder if the logger can be used to create a stdout-based progress-bar-like output like I have in this PR. The point is not really to log anything for posterior use, but to give the user a real-time visual feedback on the fit's progress, while it's happening. The stdout output is not meant to be kept, or used for anything else, much less for science. It may lack the numerical precision available in the output table, as well as have incomplete information. The logger solution seemed to me to be an overkill for just visual feedback purposes. Of course, we can always have both, although I didn't find so far a use case where logging would be necessary. As @bsipocz pointed out, it would make all the sense in a pipeline or background multi-job environment, but I would advise against running the ellipse fitting algorithm in such way, since it typically needs significant "nursing" from the user.

Copy link
Member

@larrybradley larrybradley left a comment

Choose a reason for hiding this comment

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

Thanks, @ibusko

@larrybradley
Copy link
Member

@ibusko I manually merged this PR (2155dbd) because your branch disappeared and had conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants