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

Added option to use the inner product as a dissimilarity measure. #395

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

johnmarktaylor91
Copy link
Collaborator

Added an option to use the inner product as a dissimilarity measure. This may or may not be useful for users so feel free to discard this pull request, but it would sometimes be handy as a comparison with other methods when experimenting with new analysis options.

@JasperVanDenBosch
Copy link
Contributor

Thanks for this @johnmarktaylor91 ! We talked about this briefly last Tuesday and one thing that came up was how to deal with the diagonal. When you're done on your end, maybe we can have a quick chat to talk about this and tests, docs etc.? Otherwise if you prefer I can also write more here.

@johnmarktaylor91
Copy link
Collaborator Author

Apologies for the slow response, was out of town this weekend! Good point about the diagonals. My first instinct is to only include the off-diagonals (for symmetry with the other dissimilarity metrics), but could be worth thinking more about... I am free to chat between 12-3pm tomorrow (Thursday), or tomorrow afternoon, if that would be the fastest way to figure things out.

@jdiedrichsen
Copy link
Contributor

Hi John,
I think it would be great to find a way of dealing with Second-moment matrices (i.e. matrices of dot-products) as similarities within the RSA toolbox. The implementation is not 100% straightforward, given the diagnonal. I've been mostly working with those through the PCM toolbox (which has utilities for inner-product matrices), or by reconstructing the second-moment matrices from the crossnobis (or square Eucledian) distances, assuming that the mean patttern is removed.

I would be happy to join the conversation tomorrow if you guys are meeting 12-1pm (EST)....

Let me know!

Joern

@JasperVanDenBosch
Copy link
Contributor

JasperVanDenBosch commented Jun 19, 2024 via email

@johnmarktaylor91
Copy link
Collaborator Author

Hi Jasper and Joern, thanks for the comments and glad you think this could be a useful idea--maybe we can start async and if there's something that requires more discussion we can plan a Zoom meeting? Happy to handle the coding end for this.

@johnmarktaylor91
Copy link
Collaborator Author

johnmarktaylor91 commented Jun 20, 2024

Thanks again for the chat Joern--just to keep folks updated, our current thought is implement a subclass of the RDMs class (called KMs for "kernel matrix" or some such thing; name can be discussed) that inherits the functions and interface of the RDMs class, with appropriate modifications for the handling of the diagonal (for example, using the entire vectorized kernel matrix), keeping all the various "quality of life" functions for subsetting/subsampling the matrix and so forth. An alternative would be to devise an abstract base class from which both RDMs and KMs inherit, but this might be overkill. Happy to do the plumbing work for implementing this.

@JasperVanDenBosch
Copy link
Contributor

Thanks again for the chat Joern--just to keep folks updated, our current thought is implement a subclass of the RDMs class (called KMs for "kernel matrix" or some such thing; name can be discussed) that inherits the functions and interface of the RDMs class, with appropriate modifications for the handling of the diagonal (for example, using the entire vectorized kernel matrix), keeping all the various "quality of life" functions for subsetting/subsampling the matrix and so forth. An alternative would be to devise an abstract base class from which both RDMs and KMs inherit, but this might be overkill. Happy to do the plumbing work for implementing this.

This seems like the right approach; the inner product matrix is a different beast therefore it should be represented by a separate class. Not to discourage you but I think this could a fair amount of work though; think about the optimized code (cython), unbalanced, and all the downstream functions; presumably some of those would support the KM, and may have to be modified?

Aside from the separate class idea; here's some suggestions for the functionality currently in the PR:

  1. It'd be great to have unit tests for the new functionality; of the type that actually tests the computation, i.e. a very small sample of (random or simple) data, compute the expected values, run the API code, then compare the results.
  2. We should update the docs, narrative and reference, but obviously this should wait until a decision is made on the interface.
  3. If we keep the current interface we should also test (i.e. add an integration test here and there) some of the downstream functions to see what happens when they are fed the new inner product "RDMs". If they are meant to fail we should have some meaningful error message (i.e. "this operation is not supported for inner product matrices"). Maybe with TestCase.assertRaises().

I don't mean to just drop a ton of work on you, happy to help out, talk in more detail, or split this up into multiple PRs if that's more convenient. What do you think?

@nkriegeskorte
Copy link
Member

nkriegeskorte commented Jun 21, 2024 via email

@jdiedrichsen
Copy link
Contributor

Jasper, marieke and I had a discussion on this.
The vote is to introduce a superclass (Representational Matrices) that implements the common behaviors, and having the REpresentational Dissimilarity Matrices and a Representational Kernel Matrices class inherited from this

Naming suggesitons are:

Superclass: RMs
Inherited: RMDs and RKMs

Does this sound good?
J

@HeikoSchuett
Copy link
Contributor

Hi! I only just noticed this discussion, sorry for being late to the party!

Overall your discussions all seem reasonable to me. To support different summary matrices, a superclass seems to be the clean approach.

One thing to note is that this will most likely entail separate implementations of some of the convenience functions like extracting vectors/matrices subsetting etc., because variables like the diagonal, distances to 0 and/or 1 and so on will need a different treatment than RDMs. Nothing to be afraid of, I think and if we get this right the bootstrap methods and many of the comparison techniques should keep working, but I think this is good to note.

@nkriegeskorte
Copy link
Member

nkriegeskorte commented Jul 3, 2024 via email

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.

5 participants