[Clam-devel] [PATCH] (work in progress) Harmonizer

[Hernán Ordiales]

On 7/10/07, Pau Arumi <parumi at iua.upf.edu> wrote:
> En/na Hernán Ordiales ha escrit:
> > On 7/9/07, Pau Arumi <parumi at iua.upf.edu> wrote:
> >> 2) Control api is something very "core" and so it needs to be
> >> covered by unit tests. So please, try adding simple tests that
> >> checks the behaviour of SetDefault() in
> >> CLAM/test/UnitTests/ControlsTests/ControlsTest.cxx
> >> Actually unit tests not only assess that code is working, but also
> >> is a means for doing api specification.
> >
> > ok, added
>
> Commited it as is (in rev 10371). However I'd like more changes
> --it's a matter of details. Tell me if you prefer me to take it.

ok, told me These kind of changes are useful for me to learn more
about CLAM  (or are comments below?)

> > void testInControl_doesntHaveDefaultValueBydefault()
> >     {
> >         CLAM::InControl inControl("inControl");
> >         CPPUNIT_ASSERT_EQUAL( false, inControl.HasDefaultValue() );
> >     }
> >     void testInControl_defaultValue()
> >     {
> >         CLAM::InControl inControl("inControl");
> >         CPPUNIT_ASSERT_EQUAL( 0.0f, inControl.DefaultValue() );
> >     }
> >     void testInControl_settingDefaultValue()
> >     {
> >         CLAM::InControl inControl("inControl");
> >         inControl.SetDefaultValue(5.f);
> >         CPPUNIT_ASSERT_EQUAL( 5.f, inControl.DefaultValue() );
> >     }
> >     void testInControl_hasDefaultWhenTrue()
> >     {
> >         CLAM::InControl inControl("inControl");
> >         inControl.SetDefaultValue(0.f);
> >         CPPUNIT_ASSERT_EQUAL( true, inControl.HasDefaultValue() );
> >     }
>
>
> It's good to see developers starting (I assume) to be used to
> unit-testing. In this case, however, I think it doesn't cover the
> "interesting" case. When you have set bounds and also set a
> default, the set default prevails.

yes, true

> Actually I would not add a HasDefaultValue method because the
> semantics are not so clear: note that we are using DefaultValue()
> even when not HasDefaultVale(). So we always have default, but its
> value depends on the situation.
>
> My definitions would be: DefaultValue() *is* the bounds mean unless
> another default value have been fixed with SetDefaultValue()
> Note that this reduce the needed testing to only one (the one
> described before: "set default prevails to bounds")
>
> Yes, that means that I'm for eliminating the 0 as default for not
> bounded controls. Thus default would always be the bounds mean. It
> seems a simpler behaviour to me. If a processing needs a specific
> default then it should define it. If don't agree explain your
> reasons. I'm always ready to get convinced.

at first, i added that '0' return because i thought upper and lower
were not initialized and then DefaultValue() come with a wrong one (i
don't know why... but i remember once i got very big numbers in
processings InControl values as default, i think was that, but i'm not
sure now)
but isn't, they are correct, they are initialized with 0. and 1., then
default it's 0.5 if bounds were not set... its ok for me (that should
be a unit test?)

> About the method doc:
> +       /** Returns default value if exists. If don't and bounds
> were set, returns the mean between them or 0 as general default */
>          TControlData DefaultValue() const;
>
> If we agree on my proposal it would be something like :
> /** Returns the bounds mean or the value set with SetDefaultValue()
> if its the case */

agree

> >> While we are on it, you could also doxygen document the
> >> DefaultValue() method which is not totally obvious. Do you know
> >> whow to do it? hint: /** doc */
> >
> > yep ( i added doxygen comments previously in others patchs ;-) )
> > added too
>
> Oh yes indeed (momentarily lapsus).

:-)

another question, i was trying to add the configuration object to
harmonizer processing... but i'm getting this assert:

At file scons/libs/core/include/CLAM/Processing.hxx line 396
Configuring a Processing with a configuration not being the proper type.

it's about this (when i call CopyAsConcreteConfi from ConcreteConfigure):

template <typename ConcreteConfig>
inline void Processing::CopyAsConcreteConfig(ConcreteConfig &
concrete, const ProcessingConfig & abstract) const
{
	CLAM_ASSERT(typeid(ConcreteConfig)==typeid(abstract),
		"Configuring a Processing with a configuration not being the proper type.");
	concrete = static_cast<const ConcreteConfig &>(abstract);
}


I follow this guide
"http://www.clam.iua.upf.edu/wikis/clam/index.php/Processings_with_configuration"
and many others processing and i've done it in the same way (inherit
from ProcessingConfig, etc), i mean:

SMSHarmonizerConfig.hxx:
class SMSHarmonizerConfig : public ProcessingConfig
{

...

SMSHarmonizer.hxx:
...
typedef SMSHarmonizerConfig Config;
...
Config mConfig;

SMSHarmonizer.cxx:
bool SMSHarmonizer::ConcreteConfigure(const ProcessingConfig& config)
		{
			CopyAsConcreteConfig( mConfig, config );


could you help me with this?

cheers,
-- 
Hernán
http://h.ordia.com.ar
GnuPG: 0xEE8A3FE9