Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properties: some improvements #64

Merged
merged 9 commits into from
Oct 16, 2015
Merged

Properties: some improvements #64

merged 9 commits into from
Oct 16, 2015

Conversation

danyaljj
Copy link
Member

With this PR the new definitions for properties should be fully functional. Also,

Note: I still need to remove the old property definitions and change the core and existing instances accordingly. I will do these in another PR. And fixing the naming patterns inside the properties( issue #33 ) for another PR.

@danyaljj danyaljj mentioned this pull request Oct 15, 2015
@kordjamshidi
Copy link
Member

@danyaljj : Is your last PR working really?
I have this code

val listIntAttributeArrayOldWay = property[Document]("listInt") {
    x: Document => List(1)
  }
}

and then this :

val x6=listIntAttributeArrayOldWay(tests.head)

does not compile where tests.head is a Document
however this old version is working instead:

val listIntAttributeArrayOldWay = intAttributesArrayOf[Document]('listInt) {
    x: Document => List(1)
  }

@danyaljj
Copy link
Member Author

@kordjamshidi @christos-c @sameersingh This PR is ready for review. Comments are welcome!

@danyaljj danyaljj changed the title [WIP] Properties: removing old definitions Properties: removing old definitions Oct 15, 2015
@danyaljj danyaljj changed the title Properties: removing old definitions Properties: some improvements Oct 15, 2015
@kordjamshidi
Copy link
Member

Have you run an example with mixed features?

@danyaljj
Copy link
Member Author

No, any problem?

@danyaljj
Copy link
Member Author

@kordjamshidi have you noticed any issue?

@@ -8,15 +9,31 @@ class propertyTest extends FlatSpec with Matchers {
"properties" should "work!" in {
import toyDataModel._
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kordjamshidi I have this unit test in which I test all of the properties.

@kordjamshidi
Copy link
Member

I think it just did not work, even in Lbjava.

@danyaljj
Copy link
Member Author

What file? Where? What is the error?

@kordjamshidi
Copy link
Member

I have not run it to see what is the error, but I guess the mixed attributes should be checked as those did not work. You can try an example in Lbjava first. I guess the code in this file: saul/saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/datamodel/attribute/CombinedDiscreteAttribute.scala
and also RelationalFeatures in the same location should be relevant to this.

@kordjamshidi
Copy link
Member

@danyaljj , does this version work when calling properties on instances? I need a working version, and I am going to accept this PR if this is working-- because your previous PR does not work.

@danyaljj
Copy link
Member Author

As I mentioned it removes a bug I had in the previous version.
And you can see I have a unit test for properties in which all of them pass.
If you see a new bug, you should let me know. Otherwise, I won't notice it.

@kordjamshidi
Copy link
Member

Sorry if I miss something here, it would be helpful if you point me to the place in your test unit that calls the property by its name and directly applies on an instance like the example I mentioned:
listIntAttributeArrayOldWay(tests.head)

@danyaljj
Copy link
Member Author

I am coming to your office!

@kordjamshidi
Copy link
Member

This is ok for me to merge now.

kordjamshidi added a commit that referenced this pull request Oct 16, 2015
@kordjamshidi kordjamshidi merged commit cbeeca7 into CogComp:master Oct 16, 2015
kordjamshidi pushed a commit to kordjamshidi/saul that referenced this pull request Mar 13, 2017
Moved to a Phrased based Data model
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants