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

More BLeSS efficiencies #434

Merged
merged 9 commits into from
Mar 29, 2024
Merged

More BLeSS efficiencies #434

merged 9 commits into from
Mar 29, 2024

Conversation

davidcerny
Copy link
Contributor

Some additional improvements to the performance of ln(pdf), again following very helpful suggestions by Claudio Kozický (@qak). The main changes are as follows:

(1) The "alignment" of the average distance matrix and the distance matrix of a proposed tree (i.e., the step of ensuring that we are subtracting distances corresponding to the same pair of taxa) is done using std::sort rather than std::find, which is up to two orders of magnitude faster for 10,000-by-10,000 matrices;

(2) Indexing is done using uint32_t rather than size_t, which speeds up memory access (and consequently, the matrix subtraction step).

In practice, the performance improvement gained by these changes can be difficult to observe, because the total amount of time spent on evaluating the (log) probability density of a proposed tree is now dominated by the time it takes to convert the tree to a distance matrix. However, some experimentation with std::chrono::steady_clock will show that the ln(pdf) calculation implemented in this branch is about 12% faster for 10,000-by-10,000 matrices (~0.63 vs. ~0.55 s).

@davidcerny davidcerny changed the title More bless efficiencies More BLeSS efficiencies Mar 19, 2024
@davidcerny davidcerny requested a review from hoehna March 26, 2024 17:49
hoehna
hoehna previously requested changes Mar 28, 2024
Copy link
Member

@hoehna hoehna left a comment

Choose a reason for hiding this comment

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

Looks good, the only suggestion I have is to move the string index sort to StringUtilities so that it can be used more broadly.

@@ -21,6 +23,28 @@

using namespace RevBayesCore;

/*!
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense to add this function to StringUtilities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point – moved it there!

@bredelings bredelings dismissed hoehna’s stale review March 29, 2024 18:52

The requested changes were implemented.

@bredelings bredelings merged commit ed422ba into development Mar 29, 2024
20 checks passed
@davidcerny davidcerny deleted the more-bless-efficiencies branch March 29, 2024 18:53
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.

None yet

3 participants