search

ELVIS-Project / vis-framework

Thoroughly modern symbolic musical data analysis suite.
http://elvisproject.ca/
31 stars 6 forks source link

Make ngram_indexer take its horizontal interval readings from the first offset rather than the second. #352

Closed alexandermorgan closed 8 years ago

alexandermorgan commented 9 years ago

The ngram_indexer strings together adjacent events and connects them with the readings of the horizontal interval indexer. This is done by taking the horizontal interval indexer reading at the offset of the second event in any step of the ngram. To make the results correct, the horizontal interval indexer has to have the horiz_attach_later setting set to True, i.e. the horizontal interval indexer results provide the melodic interval that was just taken to get to the note at the offset in the index, rather than saying what horizontal interval will occur next. This mix of settings is prone to user error as people could easily run the horizontal interval indexer with the default horiz_attach_later=False, and then feed those results into the ngram_indexer, and therefore produce nonsensical results.

crantila commented 9 years ago

If you're going to start refactoring the NGramIndexer, maybe we should rewrite it from scratch? I've been meaning to do that for a long time, but there was never a particular from the professors. Could we work on this together?

alexandermorgan commented 9 years ago

Sure, we could work this out later this month.

crantila commented 9 years ago

NB: if we do this, we'll also fix issue #274 at the same time.

alexandermorgan commented 8 years ago

It turns out that it is an indexing nightmare if you associate "horizontal" events with the previous point in time rather than the point in time where they actually change. The new_ngram indexer therefore continues to use horizontal interval indexer results that were calculated with the 'horiz_attach_later' setting set to True.

crantila commented 8 years ago

So we can close this?

alexandermorgan commented 8 years ago

Yes, I was planning on letting a commit close it but I'm running into problems with the tests on Travis. You can close this now though if you like.

crantila commented 8 years ago

Wow, Mr Fancypants has to close his issues with a commit! :wink:

There's no rush, I was just wondering.

alexandermorgan commented 8 years ago

Associating melodic motions with the index of the first note's onset turned out to be a very bad idea. It becomes an indexing nightmare if you want to figure out what harmonic events are happening at the moment of change in the melody. This moment of change corresponds to the second note's onset.