[Clam-devel] [PATCH] segmentation in chord extractor as a seperate class

[Roman Goj]

David García Garzón wrote:
> First of all, the training we did last weeks was about you learning to go 
> through a set of little changes in a controlled way instead of changing it 
> all, compile and see what it happens. You got in trouble, you deserve 
> that ;-) 

Well, it was fun while it lasted, all those pointers and references... ;-)

It seemed the smallest change I could make... (though also seemed messy
while I was coding... :/ ) oh well, at least now I know my way around
ChordExtractor better...

> Remember, you are modifying existing, working, very sensible, and 
> hard to verify code. Be careful. You could have never reached the stable 
> state again, but rolling back changes.
> 
> Another issue is that the processing should be independent of the pool. Try to 
> construct a Segmentation object (already in clam, do not mix with the 
> Segment). Segmentation has several derivatives, choose the proper one.
> Feel free also to propose changes to the class interface.
> 
> http://clam.iua.upf.edu/doc/CLAM-doxygen/classCLAM_1_1Segmentation.html

Great, thanks! Didn't know about this... still much to learn I see :)

> After that, the ChordExtractorDescriptionDumper, should take such segmentation 
> as processing output and do the actual dump on the pool to serialize.
> 
> Each class has its own responsability.

OK, so this time maybe I'll test here whether I understand everything ok: :)

Generally speaking, the class ChordExtractorSegmentation should stay,
but taking only extractor in the constructor. Inside the code should
decide whether a new segment should be inserted (began or ended that
is). The insertion would be done into an object of type
DiscontinuousSegmentation. Then ChordExtractorSegmentation should return
the vector<double> timePositions, so that
ChordExtractorDescriptionDumper can write that to the file... right?

If right:
should ChordExtractorSegmentation segmentation be a member of the dumper
or just be declared and used in main (then the dumper would need to take
the pointer to the time positions in the constructor)? I'd say it should
live in main - seems nicer...

But the first step - what I should do now is something like:
declare a DiscontinuousSegmentation in the Dumper, first insert the new
segment onset/offset to the segmentation and then use this
vector<double> to actually write to the pool...? So that later the
construction of the vector<double> can be separated into a new class (to
do more complicated things)?

Also - another doubt I have:
Right now the segmentation code doesn't take advantage of the fact that
the chord extractor runs offline - while inserting the beggining and end
of a segment it can take into account only the frame that came before
the current frame. Should I (step 2?) move this code i.e. to the
destructor of the ChordExtractorDescriptionDumper and make it work on
the whole song at once? (at first of course doing nothing more than is
done now) ... or maybe should I focus on doing it realtime, since this
is what is needed in the NetworkEditor?

Heh, I'm not sure I really made myself clear... maybe I should have just
coded this and asked whether it's ok... it would have taken me a similar
amount of time and would have been immediately clear ;-)

Anyway, thanks for the comments, eagerly waiting for more! :-) (and in
the meantime I'll try the first step as described above)

cheers,
roman

> On Thursday 19 July 2007 11:42:34 Roman Goj wrote:
>> Hi!
>>
>> I've started working on chord segmentation - starting offline in
>> ChordExtractor. The first step, suggested by David: separating the
>> code responsible for segmentation, so that it is all in a separate
>> class.
>>
>> In short:
>> Changelog:
>> * Most of the segmentation code in ChordExtractor is now in a seperate
>> class called ChordExtractorSegmentation
>>
>> This patch is a "blind man's approach" to the problem - the simplest
>> first patch I could think of... I just took the biggest chunk of code
>> responsible for segmentation, put it into the doIt of a new class and
>> added the constructor. Then I added some other code to doIt, so that
>> every variable necessary for the copy/pasted chunk to work has the
>> same value as it had in the previous code (basically copy/pasting some
>> other lines from the code, without changing them)... then I struglled
>> with the "which variable needs to be a reference, which a
>> pointer"-mess... ;) And then finally I started getting identical
>> results from the old ChordExtractor and the new "improved and much
>> more messy" one.
>>
>> TODO:
>> * Clean up! There is some repetitive code now, probably completely
>> unnecessary (also - the old code isn't deleted, it's just commented
>> out for now). I decided to post the patch without the cleanup, so
>> that, if I'm doing something really fundamentaly wrong, someone can
>> notice now :)
>> * Quite a few variables are now passed to the constructor, probably
>> completely unnecessary (like _lastChord), this needs to change
>> * there is also some code in the destructor of
>> ChordExtractorDescriptionDumper, this needs to go into the new class
>> * lots more :)
>>
>> cheers,
>> roman
> 
> 
>