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 blob detection #11

Merged
merged 1 commit into from Jun 23, 2013

Conversation

Projects
None yet
2 participants
@adonath
Member

adonath commented May 18, 2013

Initial commit of the blob detection module.

@ghost ghost assigned adonath May 18, 2013

from scipy.ndimage import gaussian_filter, gaussian_laplace, maximum_filter
from scipy.ndimage.morphology import binary_erosion
__all__ = ['create_scale_space']

This comment has been minimized.

@cdeil

cdeil May 18, 2013

Member

List all things that a normal user should see here (this e.g. determines what is put automatically in the docs).

@cdeil

cdeil May 18, 2013

Member

List all things that a normal user should see here (this e.g. determines what is put automatically in the docs).

#Compute overlap area. Reference: http://mathworld.wolfram.com/Circle-CircleIntersection.html (04.04.2013)
else:
area = (blob.radius**2 * arccos((d**2 + blob.radius**2 - self.radius**2)/(2 * d * blob.radius))

This comment has been minimized.

@cdeil

cdeil May 18, 2013

Member

Such long formulas are almost impossible to read.
Please split it into parts using temp variables.

@cdeil

cdeil May 18, 2013

Member

Such long formulas are almost impossible to read.
Please split it into parts using temp variables.

def read_scale_cube_fits(filename):
"""Read scale space from fits file image cube"""
from astropy.io import fits

This comment has been minimized.

@cdeil

cdeil May 18, 2013

Member

Indentation error?

@cdeil

cdeil May 18, 2013

Member

Indentation error?

###Conversion functions###
def write_fits_table():

This comment has been minimized.

@cdeil

cdeil May 18, 2013

Member

Doesn't do anything in this form. Change or remove.

@cdeil

cdeil May 18, 2013

Member

Doesn't do anything in this form. Change or remove.

import logging
logging.basicConfig(level=logging.INFO)

This comment has been minimized.

@cdeil

cdeil May 18, 2013

Member

Define __all__ here and list all functions users should see in the docs and use.

@cdeil

cdeil May 18, 2013

Member

Define __all__ here and list all functions users should see in the docs and use.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil May 18, 2013

Member

Can you please add a few things before we merge?

  • some documentation to docs/tevpy/detect? At least add one sentence and the autoapi directive to generate the API docs (see e.g. docs/tevpy/stats/index.rst as an example.
  • a command line tool to run blobdetect in scripts?
  • docstrings in this format. For you it's clear what each function does, but for me and all others we need some info e.g. what the inputs and outputs are. Internal functions that are not listed in __all__ = [...] and don't appear in the docs don't need to be so well documented.
  • add unit tests that run an example. Since I guess you have no "verified test case" maybe you can use
import numpy as np
np.random.seed(0) # to have reproducible output
image = np.random.randn((200, 100))

and a low threshold to detect some blobs? This at least shows that the code runs and prevents regressions.

If you can't figure out how to do something, look for examples in astropy or photutils or ask here.

Member

cdeil commented May 18, 2013

Can you please add a few things before we merge?

  • some documentation to docs/tevpy/detect? At least add one sentence and the autoapi directive to generate the API docs (see e.g. docs/tevpy/stats/index.rst as an example.
  • a command line tool to run blobdetect in scripts?
  • docstrings in this format. For you it's clear what each function does, but for me and all others we need some info e.g. what the inputs and outputs are. Internal functions that are not listed in __all__ = [...] and don't appear in the docs don't need to be so well documented.
  • add unit tests that run an example. Since I guess you have no "verified test case" maybe you can use
import numpy as np
np.random.seed(0) # to have reproducible output
image = np.random.randn((200, 100))

and a low threshold to detect some blobs? This at least shows that the code runs and prevents regressions.

If you can't figure out how to do something, look for examples in astropy or photutils or ask here.

def q_factor(self, blob):
"""Compute q factor as overlap criterion"""
#Approx HESS PSF size

This comment has been minimized.

@cdeil

cdeil May 18, 2013

Member

This shouldn't be hardcoded to an instrument-specific value. Make it an option, probably best without default value because there is none that will work for many users.

@cdeil

cdeil May 18, 2013

Member

This shouldn't be hardcoded to an instrument-specific value. Make it an option, probably best without default value because there is none that will work for many users.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil May 18, 2013

Member

And can you please clean up the code a bit to follow PEP8?

The pep8 tool to check and autopep8 tool to automatically clean up are your friends.

Some things I noticed but didn't comment on:

  • a few times the indentation is off (should always be spaces only with indent 4, 8, 12, ...)
  • After a comment hash there should be a space, i.e. # comment, but you use #comment
  • Sometimes your lines are very long ... you don't have to fix all the PEP8 warnings about long lines,
    but in a lot of cases you can improve readability by splitting across multiple lines.
Member

cdeil commented May 18, 2013

And can you please clean up the code a bit to follow PEP8?

The pep8 tool to check and autopep8 tool to automatically clean up are your friends.

Some things I noticed but didn't comment on:

  • a few times the indentation is off (should always be spaces only with indent 4, 8, 12, ...)
  • After a comment hash there should be a space, i.e. # comment, but you use #comment
  • Sometimes your lines are very long ... you don't have to fix all the PEP8 warnings about long lines,
    but in a lot of cases you can improve readability by splitting across multiple lines.
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jun 23, 2013

Member

Merging this in ... will work on the comments I mentioned above later.

Member

cdeil commented Jun 23, 2013

Merging this in ... will work on the comments I mentioned above later.

cdeil added a commit that referenced this pull request Jun 23, 2013

@cdeil cdeil merged commit 2599f62 into gammapy:master Jun 23, 2013

1 check failed

default The Travis CI build failed
Details

@cdeil cdeil changed the title from Added blobdetection module to Add blob detection module Apr 8, 2015

@cdeil cdeil added the feature label Apr 8, 2015

@cdeil cdeil added this to the 0.1 milestone Apr 8, 2015

@cdeil cdeil changed the title from Add blob detection module to Add blob detection Apr 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment