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

[David García Garzón]

On Thursday 19 July 2007 20:04:30 Roman Goj wrote:
> 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. 

Indeed, that's not a good solution. Normally processings are decoupled one 
each other unless one includes other in a composition (like ChordExtractor 
with all its subprocessings). Processings should be coupled just to the input 
data. I mean, you should pass to the segmentator the output of the extractor 
on each doIt call as parameter, just as most small processings do. Who does 
that? Someone who has access to both processing, several candidates. Indeed 
once your only coupling is the data, you can move the processing up and down 
which is our target. Once you have it decoupled you could move the 
segmentator inside the ChordExtractor.

In summary: 

Goal 1 remove reference to the pool (after each step compile test and patch):
* Convert currentChord and _lastChord from string into indexes (should be 
equivalent, and 0~None). This will simplify all the rest of the work.
* Create a vector of time positions and another of chord indexes as members
* Populate them as the chordSegmentation and pool. (do not remove the old 
code)
* Move the old pool code to the destructor but now looping and translating one 
to the other.
* Add accessors to the new vectors
* Move the old pool code loop to the destructor of the Dumper using such 
accessors.
* Remove all pool dependant code
* I don't know if it would be usefull switching to the Segmentation at this 
stage. Let's ignore that by now.

Goal 2 remove reference to the segmentator (after each step compile test and 
patch):

* Add a doIt parameter to pass the chordCorrelation as parameter, and pass it 
on the segmentator::doit caller taking it from the ChordExtractor
* Do the same with first and second candidate
* Do the currentChord logic just with indexes instead strings to avoid other 
dependencies on the get fullname
* Move the pool work out of the processing (this would also 
* Remove the reference to the extractor as member



> 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
>
> _______________________________________________
> Clam-devel mailing list
> Clam-devel at llistes.projectes.lafarga.org
> https://llistes.projectes.lafarga.org/cgi-bin/mailman/listinfo/clam-devel