search

CogComp / saul

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

Some filter -> collect simplification #287

Open danyaljj opened 8 years ago

danyaljj commented 8 years ago

1) https://github.com/IllinoisCogComp/saul/blob/14ef5ff09cf222331fbdba6e5423fe8c238d277b/saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/datamodel/node/Node.scala#L62-L64

    node populate collection.filter {
      nt => property.sensor(nt.apply) == value
    }.toSeq.map(_.apply)

can be simplified to 

    node populate collection.collect { 
      case nt if property.sensor(nt.apply) == value => nt.apply 
    }

2) here: https://github.com/IllinoisCogComp/saul/blob/14ef5ff09cf222331fbdba6e5423fe8c238d277b/saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/datamodel/node/Node.scala#L291 

instances.filter(matcher(_, b)).map(_ -> b)

can be simplified to 

instances.filter(case sth if matcher(sth, b) => sth -> b)

Similarly here: https://github.com/IllinoisCogComp/saul/blob/14ef5ff09cf222331fbdba6e5423fe8c238d277b/saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/datamodel/node/Node.scala#L299

kordjamshidi commented 8 years ago

I leave this to @sameersingh, but just to mention that I am not sure why this is a simplification? and since this is backend code writing it in a different way what advantage it can have. I believe for the backed code efficiency and readability is important rather than compactness.

kordjamshidi commented 8 years ago

Yet another point, I am not sure if this is important and should be tested: in the first change above, after filter the similar filtered cases are removed since you stay as a set and do not convert to Sequence.

danyaljj commented 8 years ago

My main goal by this changes was efficiency. I believe the modifications make them faster and less memory intensive. I later discussed it further with other people, which led to good comparisons.

I will try to do some timing analysis on some examples and compare them.