Skip to content

Conversation

MihaiSurdeanu
Copy link
Contributor

No description provided.

@MihaiSurdeanu MihaiSurdeanu requested a review from kwalcock April 5, 2025 16:29
@MihaiSurdeanu MihaiSurdeanu self-assigned this Apr 5, 2025
@MihaiSurdeanu MihaiSurdeanu merged commit 6f45c7d into main Apr 5, 2025
1 check failed
@MihaiSurdeanu
Copy link
Contributor Author

@kwalcock : I added you as reviewer here and I'd be happy if you are willing to continue keeping an eye on these projects. I see you as a key contributor to these projects, which will continue as open source. But, of course, this is completely optional!

Copy link
Member

@kwalcock kwalcock left a comment

Choose a reason for hiding this comment

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

Some suggestions will be there momentarily in a PR.

Comment on lines +24 to +34
if(nonLin.isDefined) {
for (matrix <- outputs) {
for (i <- 0 until Math.rows(matrix)) {
val row = Math.row(matrix, i)
for (j <- 0 until Math.cols(matrix)) {
val orig = Math.get(row, j)
Math.set(row, j, nonLin.get.compute(orig))
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we don't have a map method. It might be nice to hide these details and maybe make it more efficient with code like

  def map(matrix: MathRowMatrix, f: MathValue => MathValue): Unit = {
    val iterator = matrix.iterator(true, 0, 0, matrix.getNumRows, matrix.getNumCols)
    
    while (iterator.hasNext) {
      val oldValue = iterator.next()
      val newValue = f(oldValue)

      iterator.set(newValue)
    }
  }

Then the Encoder would have

    nonLinOpt.foreach { nonLin =>
      outputs.foreach { matrix =>
        Math.map(matrix, nonLin.compute)
      }
    }

There will be an example PR with other details soon.

@@ -0,0 +1,15 @@
package org.clulab.scala_transformers.encoder

import org.clulab.scala_transformers.encoder.math.EjmlMath.MathValue
Copy link
Member

Choose a reason for hiding this comment

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

There's a way to make this independent of EjmlMath.

val row = Math.row(matrix, i)
for (j <- 0 until Math.cols(matrix)) {
val orig = Math.get(row, j)
Math.set(row, j, nonLin.get.compute(orig))
Copy link
Member

Choose a reason for hiding this comment

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

If this does stay, the nonLin.get might be moved outside the triply nested loops.

@kwalcock
Copy link
Member

kwalcock commented Apr 6, 2025

PS I'm not sure what happened to sbt that is preventing testing. I'll try to check up on it.

@MihaiSurdeanu MihaiSurdeanu deleted the nonlinearity branch April 6, 2025 19:46
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.

2 participants