search

ELVIS-Project / vis-framework

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

Logic in run method of offset.py not tested and it is unclear if it always starts at the right offset. #356

Closed alexandermorgan closed 8 years ago

alexandermorgan commented 9 years ago

There are no tests for the logic in the run method of offset.py and I can't tell if it deals with pick-up measures correctly. If it isn't handling them correctly, a proposed solution would be to start at the offset of measure 1 (this information would be available in the measure indexer in metre.py, though this indexer needs to be completed and tested as well) and then if there's a measure zero (which is what music21 calls a pick-up measure) go back as many offset values as possible in that pick-up measure (note that these offset values are the float assigned to the 'quarterLength' setting in offset.py). @crantila, I'm assigning this to myself but I thought you may be interested in following the issue too.

crantila commented 9 years ago

Well how about that. It looks like a... somewhat comprehensive... suite of integration tests but, sure enough, no unit tests. Maybe a good start is to write the integration test(s) necessary to figure out the situations in which it doesn't work? Unless you've already spotted some errors, or you had a different plan!

alexandermorgan commented 9 years ago

The only case I can think of that may be problematic is one in which a pick-up measure contains an amount of beats that is not wholly divisible by the user-supplied offset (i.e. the setting 'quarterLength'). For example, if you're looking at notes on every quarter note in a 3/4 time signature, and the piece starts with an eighth note pick-up (or a dotted quarter pick-up, etc) then I would want to make sure that the piece is still looking at every quarter note starting on the beat, rather than displaced by an eighth note. We just need a piece with a pick-up measure to test, and then pass it an offset interval that doesn't correspond. Also, note that I'm not saying that this doesn't work, I'm just saying I'm not confident that it does.

alexandermorgan commented 9 years ago

Closely related to issue #173

alexandermorgan commented 9 years ago

Note: tests have been added to replace the ones that were erased, but this issue remains open for the time being because there is still no way to work back from the downbeat of measure 1 through as many full offset intervals in measure zero (if there is one) as possible.

alexandermorgan commented 9 years ago

The newly refactored measure indexer from this commit d8c6ac3129ae66778bc9a04fa53951cea464af98 provides the information necessary to treat cases that have a pick-up measure.