CogComp / saul

Saul : Declarative Learning-Based Programming
Other
64 stars 18 forks source link

Property Refactoring #403

Closed bhargav closed 8 years ago

bhargav commented 8 years ago

Note: Some unit tests currently fail because the models need to be re-trained. Will train models after the PR is reviewed at least once.

kordjamshidi commented 8 years ago

"Decoupling Property and LBJava's classifier." is that something we wanted to do?

bhargav commented 8 years ago

With this PR, Property type still uses FeatureVector and Feature types from LBJava.

The main idea was that moving away from LBJava types would help integrating other algorithms -- SL Integration, Weka, etc. We should have our own data structures/interfaces for Properties and Classifiers. We can discuss this further sometime. This PR still uses LBJava's types and removes duplicated code from Property sub-classes.

danyaljj commented 8 years ago

I like the changes. It moves us in the direction of decoupling LBJ classifier and properties/sensors. This will let us to make the implementation of properties more efficient and cleaner. Later we can decide whether we want to keep using LBJava's feature extraction or have our own.

Semaphore is unhappy; SRL probably needs retraining?

Good to merge by me when Semaphore and Parisa are happy.

danyaljj commented 8 years ago

One minor comment about upper-bounds in the unit tests. Otherwise looks good.

danyaljj commented 8 years ago

@kordjamshidi do you have any comments on this PR? If not, I think we can merge this.

kordjamshidi commented 8 years ago

Give me time until tomorrow, will look at it.

kordjamshidi commented 8 years ago

I was looking into this. It would be good if we can also discuss this tomorrow before merging.

bhargav commented 8 years ago

Sure. That works.

danyaljj commented 8 years ago

Update it with IllinoisCogComp/master?

kordjamshidi commented 8 years ago

@bhargav : I think we decided on some documentation before merging this, I just noticed you have merged this.

kordjamshidi commented 8 years ago

Sorry, @danyaljj.

danyaljj commented 8 years ago

Bhargavi added some comments about the removal of the classifier function before merging. What other documentation did you have in mind?

kordjamshidi commented 8 years ago

ah sorry, so, I could not find that commit in the list then.