[Clam-devel] patch to be - tonal analysis with configuration

[David García Garzón]

On Thursday 05 July 2007 21:44:05 Roman Goj wrote:
> Hi!
>
> I'm back to working on CLAM full time afer an unexpectedly long break...
>
> I'm trying to add configuration to Tonal Analysis - the patch is
> attached (and a new file for the NetEdit source tree). I'm not adding
> [PATCH] to the subject as I'm not really sure this is the way to
> approach this... counting on some feedback :)
>
> (Of course I understand this will have to wait? Good luck tomorrow for
> the conference :) )
>
> What this patch does:
> * takes all the variables passed to the Simac::ChordExtractor and makes
> the same variables available as configuration for NetworkEditor
> * adds a reconfigure() method to Simac::ChordExtractor and
> ConstantQTransform, which basically goes through a similar process as
> when running the constructors for those classes
> * changes the .clamnetwork files appropriately (did I forget any?)
>
> What it doesn't do even though it should:
> * all the parameters except the ones used by ConstantQTransform don't
> get really affected by any changes in the configuration within the
> network (simply the reconfigure method of ChordExtractor doesn't do
> anything other then call reconfigure() and sparsekernel() for
> ConstantQTransform)
>
> It all seems to be working, I've tried changing the parameters and I
> guess the results are as expected (for example I can force the algorithm
> to ignore the lower frequency notes... to some extent only though). I
> can't really get lower then about 70-80 Hz as the min. frequency, then
> things start to go wrong... maybe the fourier transform length would
> have to get a new size? I should investigate what's happening here...
> for now at least nothing crashes.
>
> OK, now the questions, loads of them :) I may forget some here...

Comments about the patch on the reply to your next message.

> 1. This seems a very temporary method of accomplishing configuration for
>  TonalAnalysis... all the classes that TonalAnalysis uses are supposed
> to become composite processings, did I understand this correctly? A
> future TODO for me? Maybe I should start with this, is there something I
> won't be able to figure without the tutorial in the making? This way I
> guess I wouldn't have to write seperate reconfigure() methods where
> there soon should be official ConcreteConfigure :) (though adding simple
> reconfigure helps me to understand things better and should be easy to
> convert to ConcreteConfigure)

Yes, it is a future TODO. But by now, we are saving some cycles by not doing 
the way CLAM does.

> 2. Maybe this is a silly or even annoying question (sorry :( ), but are
> there any rules in naming convetions I should obey? The page on the wiki
> about this is deprecated and anyway it seems to me the Tonal code at
> least doesn't stick to it anyway... Can I try to clean up the code a bit
> and make it obey the conventions more? Ie. the member variables are
> often called _foo - this should be mFoo, am I wrong? or sometimes
> they're simply called foo, which gets hard when I need to write:
>
> void func(ifoo)
> {
> 	foo = ifoo;
> }
>
> instead of, as it seemed to me most of the code did:
>
> void func(foo)
> {
> 	_foo = foo;
> or even
> 	mFoo = foo;
> }
>
> I'm very sorry for bringing this up... but it would really be easier to
> undertand things for me if I could use one naming convention...

A larger guide will appear in short on the wiki. Just a rule of thumb until 
then: keep the naming convention on the file you are editing.

> Also cosmetic changes - I changed the long list of declarations of
> members of ChordExtractor so that each name is nicely aligned. Can I
> continue making changes like this, or are there reasons things are as
> they are now?

I prefer not doing so, because it makes harder to change the declarations. For 
example a search and replace won't keep the alignment. But i don't really 
mind if you do.

> Also - could I add more comments...? Somehow I feel at a loss even in my
> own code if I don't have the comments... they usually help me get around
> the source faster (I've no idea why, actually :/ ) ...but I guess it
> might be annoying for some to see comments taking the code's space?

We have also another rule of thumb (by Fowler): If you need comments to 
understand the code, the problem is not the lack of comments, it's about the 
code. Normally you can substitute a comment by a variable renaming or by a 
method extraction.

// This variable holds that
double p=0.5;
by
double that=0.5

// This code does that
for (unsigned i=0;....
by
void DoThat(parameters by (const?) reference)
{
	for (unsigned i=0;....
}


> 3. This question should've been first probably. I don't know how to make
> it possible to set the config variables in the NetworkEditor to values
> lower then 0.01. Example:
> I put constantQKernelThreshold as a configuration variable, normally
> it's default value is 0.0054, but NetworkEditor shows it as 0.01 anyway
> and I can't make it lower then that.
> Am I missing something obvious? Where can I change this?


This is a current limitation on the widget we are using. We have no clue on 
the needed precision for a given parameter. Let's be practical, do we really 
want the user to modify that parameter, I think we are exporting too much 
detail. If it gives us problems, it can just be an inner constant.