cca_zoo icon indicating copy to clipboard operation
cca_zoo copied to clipboard

cca_zoo.models.GRCCA.fit's keyword-argument group should default to single-feature groups

Open JohannesWiesner opened this issue 3 years ago • 2 comments

Hi James,

I want to use cca_zoo.models.GRCCA for my dataset. Both X an y can be divided into n groups. If I read the documentation correctly information on feature subsets can be provided using the groups keyword argument in the .fit() method? This keyword argument defaults to None. Just for testing purposes, I tried using .fit() without providing a value for groups to see what happens but I got this error:

File "/tmp/ipykernel_46235/3223454239.py", line 8, in <cell line: 8>
    estimator.fit([brain_df,behavior_df])

  File "/home/johannes.wiesner/.conda/envs/csp_wiesner_johannes/lib/python3.9/site-packages/cca_zoo/models/_prcca.py", line 131, in fit
    views, idxs = self.preprocess(views, groups)

  File "/home/johannes.wiesner/.conda/envs/csp_wiesner_johannes/lib/python3.9/site-packages/cca_zoo/models/_prcca.py", line 145, in preprocess
    for view, group, mu, c in zip(views, groups, self.mu, self.c)

TypeError: 'NoneType' object is not iterable

It seems like you have not implemented a solution for .fit(groups=None)? Looking back at @ElenaTuzhilina's code, it seems that she wrote the code in such a way, that if not otherwise provided, each feature forms a single group. Another solution would be to simply make groups a positional argument, where the user has to provide groups. But I guess this solution would then deviate more from the original implementation.

JohannesWiesner avatar Oct 07 '22 14:10 JohannesWiesner

Thanks @JohannesWiesner - to be honest I've documented both PRCCA and GRCCA really badly sorry. Will take a look but first glance I think you're right. Think I'll go with the best/worst of both worlds and default to none with the same flow as Elena but with a warning that says it's probably a bad idea

jameschapman19 avatar Oct 07 '22 14:10 jameschapman19

Yippie, sounds good!

but with a warning that says it's probably a bad idea

Please use this exact phrase for the warning! 😂

JohannesWiesner avatar Oct 07 '22 14:10 JohannesWiesner