ViCCo-Group / frrsa

Python package to conduct feature-reweighted representational similarity analysis.
https://www.sciencedirect.com/science/article/pii/S105381192200413X
GNU Affero General Public License v3.0
27 stars 3 forks source link

set number of parallel jobs; fix #23 #29

Closed hahahannes closed 2 years ago

hahahannes commented 2 years ago

ah yes, thanks for spotting! that implementation was bit rushed @PhilippKaniuth

PhilippKaniuth commented 2 years ago

Thanks! But line 350 would still throw an error, right? It would try to check whether the string is bigger than 1. This would throw a TypeError as the operand > is not supported between str and int.

And in 297 parallel is still declared and int? Default value of parallel in frrsa is also an int. Or am I being thick here?:D

hahahannes commented 2 years ago

I moved the conversion to line 190. Then start_outer_cross_validation expects only integers (no max anymore). Thought this might be cleaner instead of str/int type checking in start_outer_cross_validation

PhilippKaniuth commented 2 years ago

Apologies, I didn't realize that, that's nice.

However, we can't unambiguously indicate the type then, right? str is wrong if the user doesn't input "max", and int is wrong if s/he does input "max". And if the user inputs say 2 as a string (since we declared it to be input as a string) there will be the TypeError.

While I agree that the conversion is 190 is nice, I think I would favor making it as easy as possible for the user; which would be to unambiguously indicate the type as str and type check it and/or transform it once. What do you think? It could be done easily be amending the if-statement in 190 if I am not missing sth.:

if parallel == 'max':
  parallel = psutil.cpu_count(logical=False)
else:
  parallel = int(parallel)
hahahannes commented 2 years ago

aggree! otherwise the documentation would be wrong