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

Fix vector magnitude caching #142

Merged
merged 2 commits into from
Apr 9, 2015
Merged

Fix vector magnitude caching #142

merged 2 commits into from
Apr 9, 2015

Conversation

richardpoole
Copy link
Contributor

A typo in Vector.magnitude() was preventing it from returning the cached value

A typo in `Vector.magnitude()` was preventing it from returning the cached value
@benpickles
Copy link
Contributor

Fancy adding a test to prevent this from happening again?

@richardpoole
Copy link
Contributor Author

Not sure how I'd write a test for this since the method returns the correct value. I guess you could compare execution time between the first and subsequent calls, but that seems fragile.

@benpickles
Copy link
Contributor

I guess that its output isn't covered by any of the existing tests is the problem.

@richardpoole
Copy link
Contributor Author

@benpickles
Copy link
Contributor

Ah yes, but not the caching. Oh well! I'll go back to sleep now 😴

@satchmorun
Copy link
Contributor

I saw that yesterday as well, but also noticed another thing: The insert method doesn't invalidate the cache when it adds new Nodes.

Maybe that's ok, since Vectors seem to be mostly one-use, but in cases where they might not be, the typo actually prevents stale cache issues.

@richardpoole
Copy link
Contributor Author

Thanks @satchmorun. I've added cache invalidation to Vector.insert() and a corresponding test.

@olivernn olivernn merged commit 90335d3 into olivernn:master Apr 9, 2015
@olivernn
Copy link
Owner

olivernn commented Apr 9, 2015

Tpying is hrd! Good spot, wish there was a better way to prevent my fat fingered mashing of the keyboard resulting in bugs like this, any ideas?

https://www.youtube.com/watch?v=OqjF7HKSaaI

I've released version 0.5.8 with this change in, let me know if there are any issues, and thanks again for the pr.

@richardpoole
Copy link
Contributor Author

Haha, easily done. Caching code isn't exactly easy to test. :)

We could try something like this (not tested):

var cached = function (key, func) {
  return function () {
    if (this[key]) return this[key]
    return this[key] = func.apply(this, arguments);
  }
}

lunr.Vector.prototype.magnitude = cached('_magnitude', function () {
  var node = this.list,
      sumOfSquares = 0,
      val

  while (node) {
    val = node.val
    sumOfSquares += val * val
    node = node.next
  }

  return Math.sqrt(sumOfSquares)
})

However it doesn't prevent typos in the invalidation code (e.g. this._magniture = undefined) and it makes the code harder to read. Personally, I think the risk of a typo in the caching code is the lesser of two evils.

@richardpoole richardpoole deleted the patch-1 branch April 13, 2015 17:23
@benpickles benpickles mentioned this pull request Apr 20, 2015
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.

4 participants