Skip to content

Optimize maximization functions by using a single which.max - #5

Open
chinandrew wants to merge 2 commits into
martakarass:masterfrom
chinandrew:optimize_max
Open

chinandrew wants to merge 2 commits into
martakarass:masterfrom
chinandrew:optimize_max

Conversation

@chinandrew

@chinandrew chinandrew commented May 22, 2023

Copy link
Copy Markdown

Previously, maxAndTune did 3 operations in its while loop that required scanning an entire similarity matrix:

  1. all(is.na())
  2. max()
  3. which() to find the index of the maximum found in (2)

These can all be combined into a single which.max

  1. It will return integer(0) if the matrix is all NA, which can be which can be checked to do (1)
  2. The index can be used to pull the maximum value in constant time to do (2)
  3. It returns* (3)

* - technically it returns the index as if the matrix was strung out into a vector, which you can then easily convert back to a row/column index. See the idxToRowCol function for more.

finetune_maxima also did the last 2 and is fixed as well.

For function naming, I saw camelCase (e.g. scaleTemplate), words separated by periods (e.g. get.x.smoothed), and words separated by underscores (e.g. finetune_maxima). Seemed like camelCase was most common so I went with that, though let me know if something else is preferred.

Profiled code (Full run is ~75s with compute.template.idx=FALSE on master branch):

Before:
max_before
After:
max_after

@chinandrew

Copy link
Copy Markdown
Author

Appveyor failure looks unrelated, perhaps due to something similar to this?

@muschellij2

Copy link
Copy Markdown
Collaborator

I think arrayInd is what you want - idxToRowCol seems to be recreating that.

@chinandrew

Copy link
Copy Markdown
Author

I think arrayInd is what you want - idxToRowCol seems to be recreating that.

This is great, didn't know about it. Updated the code accordingly.

Thanks for the tip.

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