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

custom clustering now takes position matrix instead of distance^2 matrix #87

Open
wants to merge 48 commits into
base: custom_cluster
Choose a base branch
from

Conversation

AdamOrmondroyd
Copy link
Contributor

This should allow more general clustering algorithms such as k-means.

@AdamOrmondroyd
Copy link
Contributor Author

Have a feeling that I've got the indices for position_matrix backwards - will try check this afternoon

Copy link
Member

@williamjameshandley williamjameshandley left a comment

Choose a reason for hiding this comment

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

Many thanks for this @Ormorod.

I've tried to catch as many of the small errors as I can in this review. I would recommend combing through with git difftool with respect to master to check to see if there are any more instances which we've missed.

pypolychord/_pypolychord.cpp Outdated Show resolved Hide resolved
pypolychord/polychord.py Show resolved Hide resolved
pypolychord/polychord.py Outdated Show resolved Hide resolved
src/polychord/clustering.f90 Outdated Show resolved Hide resolved
run_pypolychord.py Outdated Show resolved Hide resolved
src/polychord/interfaces.hpp Show resolved Hide resolved
@AdamOrmondroyd
Copy link
Contributor Author

I think i've got it working again, will now swap the indices over...

@AdamOrmondroyd
Copy link
Contributor Author

AdamOrmondroyd commented Jun 30, 2022

I'm happy that the custom clustering interface is working as intended, I've attached a double-Gaussian likelihood comparing 9 runs with either default clustering or a copy of the knn clustering algorithm written in Python passed through the interface.

wrap_cluster() increases the cluster list values by one, so that the user can provide an algorithm which numbers the clusters from 0. I'd intended to put this at the C++/Fortran join, but I couldn't see the natural place this would go. Therefore, to select default clustering, pass an array of -1.

custom_cluster_comparison

@@ -4,7 +4,6 @@ on:
push:
branches: [ master ]
pull_request:
branches: [ master ]

Choose a reason for hiding this comment

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

Could you merge PolyChord:master into Ormorod:ormorod-custom_cluster so that the diff is cleaner?

Choose a reason for hiding this comment

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

It would also be good to add a python test of clustering if you have time.

@AdamOrmondroyd AdamOrmondroyd changed the base branch from custom_cluster to master July 29, 2022 11:26
@AdamOrmondroyd AdamOrmondroyd changed the base branch from master to custom_cluster July 29, 2022 11:26
@AdamOrmondroyd AdamOrmondroyd changed the base branch from custom_cluster to master July 29, 2022 11:27
@AdamOrmondroyd AdamOrmondroyd changed the base branch from master to custom_cluster July 29, 2022 11:27
@AdamOrmondroyd
Copy link
Contributor Author

I remembered that I had a translation of the native clustering into python down the back of the sofa, so have copied that into the new example and removed the sklearn dependence. I'll remove the redundancies (I don't think num_clusters_old gets used?)

AdamOrmondroyd and others added 2 commits February 20, 2023 12:12
* neighbor spelling (assuming we're sticking to American spelling!)

* change to checkoutv4

* change to checkout@v3

* change to pythonv4

* change to ubuntu-20.04

* return to latest ubuntu for 3.7 and 3.8, and separately include 3.6 case

* include pythons up to 3.11

* enclose versions in quotes

* revert spelling changes
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