ELVIS-Project / vis-framework

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

Offset Indexer Documentation #440

Closed musicus closed 7 years ago

musicus commented 7 years ago

The docstring currently explains:

>>> from vis.models.indexed_piece import Importer
>>> ip = Importer('path_to_piece.xml')
>>> notes = ip.get_data('noterest')
>>> setts = {'quarterLength': 2}
>>> ip.get_data('offset', data=notes, settings=setts)

However, notes here is a dataframe, and the data argument for the offset indexer in the .get_data() method, actually requires a series to be provided to the data argument.

An additional step is needed in the docstring that specifies that the notes variable has to be converted to a series before it can be passed to the data argument within the .get_data() method.

musicus commented 7 years ago

Fixed with commit fbcdd50.

alexandermorgan commented 7 years ago

This is not the best fix because it doesn't make things any easier for the user and it asks the user to do something that the superclass would have done automatically (in the case of NoteRestIndexer results). It's better to put the check for the dataframe type in the init method of offset.py so that it can accept either a list of series or a dataframe. I'll go ahead and do this and adjust the documentation accordingly.

alexandermorgan commented 7 years ago

Note that the "Refactor" commit above (on develop) now allows for a dataframe to be passed, and retains level one of the dataframe's multi_index (i.e. the column names minus the indexer name) from the original passed dataframe. The change actually took place in the superclass, indexer.py, so this issue hopefully won't come up again with other indexers.

musicus commented 7 years ago

I used your bit of code, but you were able to fix the issue otherwise, so great!