{"action":"created","issue":{"url":"https://api.github.com/repos/ELVIS-Project/vis-framework/issues/416","repository_url":"https://api.github.com/repos/ELVIS-Project/vis-framework","labels_url":"https://api.github.com/repos/ELVIS-Project/vis-framework/issues/416/labels{/name}","comments_url":"https://api.github.com/repos/ELVIS-Project/vis-framework/issues/416/comments","events_url":"https://api.github.com/repos/ELVIS-Project/vis-framework/issues/416/events","html_url":"https://github.com/ELVIS-Project/vis-framework/issues/416","id":175374245,"number":416,"title":"Remove capability for recursion in IndexedPiece.get_data()","user":{"login":"alexandermorgan","id":2601689,"avatar_url":"https://avatars.githubusercontent.com/u/2601689?v=3","gravatar_id":"","url":"https://api.github.com/users/alexandermorgan","html_url":"https://github.com/alexandermorgan","followers_url":"https://api.github.com/users/alexandermorgan/followers","following_url":"https://api.github.com/users/alexandermorgan/following{/other_user}","gists_url":"https://api.github.com/users/alexandermorgan/gists{/gist_id}","starred_url":"https://api.github.com/users/alexandermorgan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alexandermorgan/subscriptions","organizations_url":"https://api.github.com/users/alexandermorgan/orgs","repos_url":"https://api.github.com/users/alexandermorgan/repos","events_url":"https://api.github.com/users/alexandermorgan/events{/privacy}","received_events_url":"https://api.github.com/users/alexandermorgan/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2016-09-06T23:14:16Z","updated_at":"2016-09-07T21:48:09Z","closed_at":null,"body":"get_data() currently supports recursive calls which allows it to queue up multiple indexer runs in one command. When more than one indexer is included in the call, it necessarily feeds the result of the nth indexer to the call to the n+1th indexer. Here's the recursive block from the code:\r\n```\r\n        if len(analyzer_cls) > 1:\r\n            if analyzer_cls[0] is noterest.NoteRestIndexer:\r\n                return self.get_data(analyzer_cls[1:], settings, data)\r\n            return self.get_data(analyzer_cls[1:], settings, analyzer_cls[0](data, settings).run())\r\n```\r\nThere are two main issues with this:\r\n\r\n1. Only the results of the last indexer are returned\r\n2. You can only feed the results of one indexer to the next indexer in the call\r\n\r\nThis second point is the most problematic because most indexers that require the results of other indexers require the results of several indexers. This was probably originally meant for interval indexing, but now the noterest indexer results are internally called by the _get_vertical_interval() and _get_horizontal_interval() methods. The only indexer that could make use of this (unless I'm mistaken) is the ngram indexer when creating ngrams based only on one input, such as durations and this is not the main use case for the ngram indexer. I think we should just limit this method to one indexer at a time which will make it easier to use, and easier to understand.\r\nAnother related issue is that get_data() can currently make repeated calls to _import_score(), even though this never actually results in a score being imported twice because of _import_score()'s internal check.\r\nIf we're going to change this it should be done for VIS 3.0."},"comment":{"url":"https://api.github.com/repos/ELVIS-Project/vis-framework/issues/comments/245430161","html_url":"https://github.com/ELVIS-Project/vis-framework/issues/416#issuecomment-245430161","issue_url":"https://api.github.com/repos/ELVIS-Project/vis-framework/issues/416","id":245430161,"user":{"login":"crantila","id":760102,"avatar_url":"https://avatars.githubusercontent.com/u/760102?v=3","gravatar_id":"","url":"https://api.github.com/users/crantila","html_url":"https://github.com/crantila","followers_url":"https://api.github.com/users/crantila/followers","following_url":"https://api.github.com/users/crantila/following{/other_user}","gists_url":"https://api.github.com/users/crantila/gists{/gist_id}","starred_url":"https://api.github.com/users/crantila/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/crantila/subscriptions","organizations_url":"https://api.github.com/users/crantila/orgs","repos_url":"https://api.github.com/users/crantila/repos","events_url":"https://api.github.com/users/crantila/events{/privacy}","received_events_url":"https://api.github.com/users/crantila/received_events","type":"User","site_admin":false},"created_at":"2016-09-07T21:48:09Z","updated_at":"2016-09-07T21:48:09Z","body":"Making me earn my salary today!\r\n\r\n> There are two main issues with this:\r\n> 1. Only the results of the last indexer are returned\r\n> 2. You can only feed the results of one indexer to the next indexer in the call\r\n\r\nNeither of these would be problems if the analyzers followed the intended behaviour, each returning a single `DataFrame` with *cumulative* results from all previous analyzers. It was originally meant for *everything* to be done in a single call to `get_data()`, separating the three phases of music analysis: prepare, calculate, and output. Note how those phases correspond to `load()`, `run()`, and `output()` in the `WorkflowManager`.\r\n\r\nUnfortunately, I never solved the \"addressed settings\" issue, I didn't implement the \"cumulative\" part of the results, and the `WorkflowManager` is too complicated to modify. Nobody else got used to the \"three phases\" idea, so you're pushing toward (in my opinion) a clumsier but simpler architecture in VIS 3. (Sure I have my misgivings, but if a design is too complicated to even *see*, never mind maintain, then it obviously wasn't the best design)!\r\n\r\nSo you're right that this recursive capability should be removed. Hopefully the explanation will help you understand how VIS 3 is fundamentally shifting the Framework's design, so there are probably other capabilities to remove too. Feel free to ask for help with specific tasks."}}
{"actor":{"display_login":"crantila"}}