- 
                Notifications
    
You must be signed in to change notification settings  - Fork 29
 
CENSURE Keypoint Detector #1
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
base: master
Are you sure you want to change the base?
Conversation
        
          
                src/censure.jl
              
                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.
Whats the correct way to check bounds on CartesianIndex?
julia> a = CartesianIndex(2, 0)
CartesianIndex{2}((2,0))
julia> b = CartesianIndex(0, 2)
CartesianIndex{2}((0,2))
julia> a >= CartesianIndex(1, 1)
false
julia> b >= CartesianIndex(1, 1)
trueI need both the cases to be false for this to work correctly.
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.
Found checkbounds(). I'll make the changes :)
| 
           @timholy I have some doubts which I have written inline as comments on the files. I have tried to make the code fast by looking at benchmarks and ProfileView(Thanks for that!) checked the type warning for most things.  | 
    
        
          
                src/censure.jl
              
                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.
BTW, can you wrap long lines? It means I have to scroll my browser window to find the "comment" button 😄.
| 
           @timholy This is ready for review :)  | 
    
        
          
                src/censure.jl
              
                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.
Will fix the indentation.
fcc9937    to
    ed82fc1      
    Compare
  
    | 
           @timholy I am changing the Keypoint type to be an alias of CartesianIndex{2} instead of the current type since it is easier for calculating descriptors (can be seen in the upcoming BRIEF descriptor) - type Keypoint
    x::Integer
    y::Integer
end Do you have a better representation in mind?  | 
    
          
 That's a good idea---you'll get a lot of very nice behavior for free that way. For future reference when you do create your own types, remember that  type KeyPoint{T<:Integer}
    x::T
    y::T
end | 
    
        
          
                src/censure.jl
              
                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.
abstract container type. How about just Array{Int}(0)?
| 
           Aside from very small points, this seems fine to me. (I'll have to play with it to really kick the tires, but it looks very promising!)  | 
    
7229c7d    to
    1bae7e9      
    Compare
  
    6813e5a    to
    ef86fc9      
    Compare
  
    | 
           @mronian You don't seem to be working on this. Can I finish it off?  | 
    
| 
           Yeah sure. It is mostly ready, you will need to update the code to the latest Julia. I did not merge it since the docs were not there.  | 
    
| 
           In filter response for octagon filter, I don't think this is correct. The error wouldn't effect the filter response though.  | 
    
| 
           The filter response was correct since I had manually checked it for a few octagons and images. You can check it again to confirm. What seems to be the error in the code?  | 
    
| 
           It should be The filter response would come out correctly because in_sum=A + D - B - C. Swapping B and C wouldn't change in_sum.  | 
    
| 
           @tejus-gupta I don't think its wrong.   | 
    
| 
           Whoops. I mistook the lines you mentioned as those from the code instead of corrections. Agreed, it is incorrect. Go ahead, @tejus-gupta 😄 Sorry for the trouble!  | 
    
Uh oh!
There was an error while loading. Please reload this page.