Issue #1042 - Cross-Product for lists of kernels #1059
Conversation
|
Hi, The code is a little complicated to read (these kind of problems can be solved in a more elegant way if done recursively). However, if it works, it doesnt really matter. My first question: Does it work? Could you create an example and post the output of it? This has to be done anyway at some point for illustration and also for unit testing. Second question: Is it memory clean? For that you will have to run your example in valgrind. I will also comment on some minor style issues in the code |
| { | ||
| CList* return_list = new CList(true); | ||
| if (!kernel_list) | ||
| { |
There was a problem hiding this comment.
We do single if statements without the {}
|
Okay, nice! Looking forward to your reply |
|
Hello, thanks for your reply! |
|
The reason I have that first loop and ended up having the code written this way was because I wanted to avoid creating new CombinedKernels on the fly and then copying the already listed kernels from the old kernels to the newly created ones. I think that would also be the case in recursion. However it would probably be more intuitive and understandable as you said. Do you prefer it the second way? If yes, don't hesitate to tell me, it won't be any trouble to change it |
|
I 've mades the changes you said. Also I've added a unit test and added a line in another one to fix a memory leak. |
| { | ||
| CList* c_list= dynamic_cast<CList* >(list); | ||
| if (!c_list) | ||
| SG_SERROR("CCombinedKernel::combine_kernels() : Failed to cast list of type " |
|
Hi! |
Issue #1042 - Cross-Product for lists of kernels
|
@van51 while I wanted to write an example showing how to use this for modelselection, I realised that you change the order of the kernels. So if the input kernel list is (a, b), then the output list's combined kernels should have subkernels in the same order (a[i],b[j]). Otherwise it gets confusing. Could you add a note to the documentation of the method that the caller gets a list which does ref-counting |
|
Here is the example that you will have to change once the ordering is fine #1066 |
|
Hello! no worries about the delay, I hope you had a nice trip! |
I haven't added a check on whether each sub-list contains kernels of the same type.
Let me know if you think it's necessary.