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

Minor update re: np.arccos #382

Closed
wants to merge 1 commit into from

Conversation

patrickmineault
Copy link
Contributor

@patrickmineault patrickmineault commented Jul 16, 2024

Requested in an email from one of the content creators:

I see you also logged another minor change to the code:

Moved from arccos of cos metric to straight cosine matrix (there was no explanation and it was confusing)

The change is from np.arccos(rdms_comp) to 1 - rdms_comp

I discussed this with Niko (cc'd) today. Both of these are distance measures, but the difference is that arccos gives us an angle and makes the whole rdm comparison a metric. That's why we had it there. I think it would be good to go back to that.

Niko and Heiko have a slide in their deck (see below) that explains what a metric is so we can actually add more explanation on that if you think that would help.

@glibesyck glibesyck mentioned this pull request Jul 17, 2024
@glibesyck
Copy link
Collaborator

Added in #383.

@glibesyck glibesyck closed this Jul 17, 2024
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