{"action":"created","issue":{"url":"https://api.github.com/repos/mmp2/megaman/issues/19","repository_url":"https://api.github.com/repos/mmp2/megaman","labels_url":"https://api.github.com/repos/mmp2/megaman/issues/19/labels{/name}","comments_url":"https://api.github.com/repos/mmp2/megaman/issues/19/comments","events_url":"https://api.github.com/repos/mmp2/megaman/issues/19/events","html_url":"https://github.com/mmp2/megaman/issues/19","id":137720493,"number":19,"title":"Refactoring","user":{"login":"jakevdp","id":781659,"avatar_url":"https://avatars.githubusercontent.com/u/781659?v=3","gravatar_id":"","url":"https://api.github.com/users/jakevdp","html_url":"https://github.com/jakevdp","followers_url":"https://api.github.com/users/jakevdp/followers","following_url":"https://api.github.com/users/jakevdp/following{/other_user}","gists_url":"https://api.github.com/users/jakevdp/gists{/gist_id}","starred_url":"https://api.github.com/users/jakevdp/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jakevdp/subscriptions","organizations_url":"https://api.github.com/users/jakevdp/orgs","repos_url":"https://api.github.com/users/jakevdp/repos","events_url":"https://api.github.com/users/jakevdp/events{/privacy}","received_events_url":"https://api.github.com/users/jakevdp/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2016-03-01T23:28:57Z","updated_at":"2016-03-04T16:45:48Z","closed_at":null,"body":"Ping @jmcq89 \r\n\r\nI've been going through the package and adding some tests of corner-cases.\r\n\r\nThere are a lot of commonalities between the four embedding classes, and a lot of code that is repeated. By writing common unit tests for these, I found a number of inconsistencies with how they handle input arguments.\r\n\r\nI think this is a case where building the four algorithms from a common base class would be *very* helpful, both in keeping the code organized & terse, and in making sure all estimators are behaving as expected."},"comment":{"url":"https://api.github.com/repos/mmp2/megaman/issues/comments/192351789","html_url":"https://github.com/mmp2/megaman/issues/19#issuecomment-192351789","issue_url":"https://api.github.com/repos/mmp2/megaman/issues/19","id":192351789,"user":{"login":"jmcq89","id":6477966,"avatar_url":"https://avatars.githubusercontent.com/u/6477966?v=3","gravatar_id":"","url":"https://api.github.com/users/jmcq89","html_url":"https://github.com/jmcq89","followers_url":"https://api.github.com/users/jmcq89/followers","following_url":"https://api.github.com/users/jmcq89/following{/other_user}","gists_url":"https://api.github.com/users/jmcq89/gists{/gist_id}","starred_url":"https://api.github.com/users/jmcq89/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jmcq89/subscriptions","organizations_url":"https://api.github.com/users/jmcq89/orgs","repos_url":"https://api.github.com/users/jmcq89/repos","events_url":"https://api.github.com/users/jmcq89/events{/privacy}","received_events_url":"https://api.github.com/users/jmcq89/received_events","type":"User","site_admin":false},"created_at":"2016-03-04T16:45:48Z","updated_at":"2016-03-04T16:45:48Z","body":"With this in mind let me know if you agree with the following:\r\n\r\n- We should have a megaman base estimator which inherits from sklearn base estimator\r\n- The base class should have an `__init__` which accepts *either* a geometry object *or* a dictionary with of geometry parameters including some or all of: adjacency_method adjacency_kwds, affinity_method, affinity_kwds, laplacian_method,laplacian_kwds.\r\n- The base class should provide a fit_geometry() method -- this method should check if it's a dictionary or a geometry object and return the fitted or existing object\r\n- The base class should provide a fit_transform() function which calls .fit() and returns .embedding_\r\n- Individual embedding classes will then offer their own `__init__` which takes arguments specific to the embedding (e.g. n_components, drop_first, etc.) its own fit() function which calls the internal embedding function(s).\r\n- Internal embedding functions should work with a fitted geometry object.\r\n\r\nThe embeddings will need to be re-written to handle the new Geometry API."}}