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

chnsCompute ignores isInit #37

Closed
JN-Jones opened this issue Mar 20, 2018 · 5 comments
Closed

chnsCompute ignores isInit #37

JN-Jones opened this issue Mar 20, 2018 · 5 comments
Labels

Comments

@JN-Jones
Copy link

https://github.com/elucideye/acf/blob/master/src/lib/acf/chnsCompute.cpp ignores the isInit-param so the options aren't returned. This seems to cause an issue with Detector::initializeOpts() and later with functions like Detector::computePyramid which try to use the options.

@headupinclouds
Copy link
Contributor

headupinclouds commented Mar 23, 2018

https://github.com/pdollar/toolbox/blob/1a3c9869033548abb0c7a3c2aa6a7902c36f39c2/channels/chnsCompute.m#L119-L136

% get default parameters pChns
if(nargin==2), pChns=varargin{1}; else pChns=[]; end
if( ~isfield(pChns,'complete') || pChns.complete~=1 || isempty(I) )
  p=struct('enabled',{},'name',{},'hFunc',{},'pFunc',{},'padWith',{});
  pChns = getPrmDflt(varargin,{'shrink',4,'pColor',{},'pGradMag',{},...
    'pGradHist',{},'pCustom',p,'complete',1},1);
  pChns.pColor = getPrmDflt( pChns.pColor, {'enabled',1,...
    'smooth',1, 'colorSpace','luv'}, 1 );
  pChns.pGradMag = getPrmDflt( pChns.pGradMag, {'enabled',1,...
    'colorChn',0,'normRad',5,'normConst',.005,'full',0}, 1 );
  pChns.pGradHist = getPrmDflt( pChns.pGradHist, {'enabled',1,...
    'binSize',[],'nOrients',6,'softBin',0,'useHog',0,'clipHog',.2}, 1 );
  nc=length(pChns.pCustom); pc=cell(1,nc);
  for i=1:nc, pc{i} = getPrmDflt( pChns.pCustom(i), {'enabled',1,...
      'name','REQ','hFunc','REQ','pFunc',{},'padWith',0}, 1 ); end
  if( nc>0 ), pChns.pCustom=[pc{:}]; end
end
if(nargin==0), chns=pChns; return; end

I believe isInit should be the C++ equivalent of the matlab condition that trigger the final line above if(nargin==0), chns=pChns; return; end.

That's probably what should happen here:

//std::cout << pChns << std::endl;
// TODO

    if (!pChnsIn.complete.has || (pChnsIn.complete.get() != 1) || IIn.empty())
    {
        // Create default structures and merge:
        {
            // top level
            Options::Pyramid::Chns dfs;
            dfs.shrink = { "shrink", 4 };
            dfs.complete = { "complete", 1 };
            pChns.merge(dfs, 1);
        }

         // <SNIP>

        //std::cout << pChns << std::endl;
        // TODO
    }

Ah. TODO :)

This lib was created as part of an effort to get a few trained models running on a phone, so only some subset of the functionality has been tested. I'm not too surprised there are a number of use case gaps. Thanks for reporting. If you have a fix, feel free to send it. Otherwise, I can take a look later. We can add some GTests to cover some of this behavior as the issues come up.

The tests are here:

TEST_F(ACFTest, ACFChannelsCPU)
{
auto detector = getDetector();
ASSERT_NE(detector, nullptr);
MatP Ich;
detector->setIsTranspose(true);
detector->computeChannels(m_IpT, Ich);
#if ACF_TEST_DISPLAY_OUTPUT
cv::Mat canvas = Ich.base().t();
WaitKey waitKey;
cv::imshow("acf_channel_cpu", canvas);
#endif
// TODO: comparison for channels:
// load cereal pba cv::Mat, compare precision, etc
ASSERT_EQ(Ich.base().empty(), false);
}

TEST_F(ACFTest, ACFchnsCompute_isInit)
{
    // some test for expected default values with isInit here
}

@JN-Jones
Copy link
Author

Don't have any proper fix as I'm handling the options on my own for now. There's a similar construct in chnsPyramid though which seems to work:
https://github.com/elucideye/acf/blob/master/src/lib/acf/chnsPyramid.cpp#L179-L183

@headupinclouds
Copy link
Contributor

To be true to the reference implementation I suppose we should aim for the following:

>> result = chnsCompute

result = 

       shrink: 4
       pColor: [1x1 struct]
     pGradMag: [1x1 struct]
    pGradHist: [1x1 struct]
      pCustom: [0x0 struct]
     complete: 1

>> result.pColor

ans = 

       enabled: 1
        smooth: 1
    colorSpace: 'luv'

>> result.pGradMag

ans = 

      enabled: 1
     colorChn: 0
      normRad: 5
    normConst: 0.0050
         full: 0

>> result.pGradHist

ans = 

     enabled: 1
     binSize: []
    nOrients: 6
     softBin: 0
      useHog: 0
     clipHog: 0.2000

>> result.pCustom is empty

>> result.complete

ans =

     1

@headupinclouds
Copy link
Contributor

>> result = chnsPyramid

result = 
       pChns: [1x1 struct]
     nPerOct: 8
      nOctUp: 0
     nApprox: 7
     lambdas: []
         pad: [0 0]
       minDs: [16 16]
      smooth: 1
      concat: 1
    complete: 1

w/ >> result.pChns same as above

headupinclouds added a commit that referenced this issue Apr 1, 2018
* support chnsCompute + isInit: see #37
* support rgb-to-gray w/ correct channel count==1 : see #36
* `*.mat` serialization testing
  + download Inria/Caltech *.mat toolbox pedestrian models from github storage using cmake/hunter_private_data
  + add simple serialization tests in test-acf GTest code (more rigorous testing needed)
  + remove unused code blocks and some debug code in test-acf.cpp
* fix a boundary case in sse gradHist() code
* remove unused conditional cv::cvtColor() code blocks in rgbConvert.cpp
* add virtual destructor to ObjectDetector.h base class
* support acf shrink (via constructor input) in the GPUACF code
* update gradhist glsl shader to highp precision
* unify header include style
* remove various comment blocks + format fixing
* add acf special member isLuv to indicate special case input LUV processing (such as in gpgpu pipeline)
* provide naive GLDetector : public acf::Detector naive detector to exercise shader code in acf-detect
  + use aglet lib for portable context
  + compute acf pyramids from input cv::Mat (in practice this would be separated for efficiency)
headupinclouds added a commit that referenced this issue Apr 2, 2018
* unify header include style

* opengl related acf fixes and …

* support chnsCompute + isInit: see #37
* support rgb-to-gray w/ correct channel count==1 : see #36
* `*.mat` serialization testing
  + download Inria/Caltech *.mat toolbox pedestrian models from github storage using cmake/hunter_private_data
  + add simple serialization tests in test-acf GTest code (more rigorous testing needed)
  + remove unused code blocks and some debug code in test-acf.cpp
* fix a boundary case in sse gradHist() code
* remove unused conditional cv::cvtColor() code blocks in rgbConvert.cpp
* add virtual destructor to ObjectDetector.h base class
* support acf shrink (via constructor input) in the GPUACF code
* update gradhist glsl shader to highp precision
* unify header include style
* remove various comment blocks + format fixing
* add acf special member isLuv to indicate special case input LUV processing (such as in gpgpu pipeline)
* provide naive GLDetector : public acf::Detector naive detector to exercise shader code in acf-detect
  + use aglet lib for portable context
  + compute acf pyramids from input cv::Mat (in practice this would be separated for efficiency)

* clang-format

* remove logging

* remove output directory command line argument

use working directory

* test data download clarification

* test data updates

* tell user to update hunter if hunter_private_data is missing
* now… assume test data is available in gauze_add_test
* update drishti-upload submodule for newer hunter version so hunter_private_data is defined in ci builds

* debug ci failure to read test image on some platforms

* remove ci debugging

* use PNG format test image

Some of the platforms in drishti-upload are built without JPEG in OpenCV, so we use PNG format for now.  The OpenCV CI configs can probably be updated and unified.
@headupinclouds
Copy link
Contributor

This should be consistent with the Matlab toolbox code now (after PR #40). I've added a couple tests to ensure that the default parameter retrieval is working and returning expected values. Closing ...

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

No branches or pull requests

2 participants