patchGNU Octave - Patches: patch #8303, vectorized parcellfun and...

 
 

patch #8303: vectorized parcellfun and pararrayfun

Submitted by:  John Hunt <huntj>
Submitted on:  Sun 02 Feb 2014 07:04:28 PM UTC  
 
Category: NonePriority: 5 - Normal
Status: NonePrivacy: Public
Assigned to: NoneOpen/Closed: Open

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 07 Feb 2014 06:44:48 PM UTC, comment #14:

Again, the "as is" stuff adds quite nothing to complexity.

I strongly disagree with "no advantage".
This not a matter of "not willing to learn". I use anonymous functions often for small problems and happy with them.
Just here with a simple change, my user code is more readable.

Forcing people to do only one way I do not like (given the small increase in complexity).
But you are right, I'm fed up arguing too; we have incompatible philosophies, so let's end it there.
No big deal. I'll just keep using my function.

Take whatever you like, it's libre !
To the very least, please consider the numerous new tests that are there to ensure the good old behavior (just add them to the bottom, no code change).

Anyway, please believe that I'm thankful for your job as a maintainer. Best regards. John.

John Hunt <huntj>
Fri 07 Feb 2014 05:01:04 PM UTC, comment #13:

Presenting the interface with the 'as is' option to the user would give him information which he does not need, and which can even distract him from learning the standard way with anonymous functions, which he can also use for other functions.

And it is just bad to provide a feature which is of no advantage.

As I thought I had already made clear twice, this particular feature will not be implemented with my support.

Actually, it doesn't make me too hopeful to get your project done in a clean way and with a reasonably limited effort if we have too argue so long already over an issue which seems quite clear to me.

Olaf Till <i7tiol>
Fri 07 Feb 2014 01:20:28 PM UTC, comment #12:

The required interface change won't require any change in existing users programs. It is just an extension. Does it alleviate your concerns ?

I find the 'as is' syntax cleaner, and less error prone,
since there are no new temporary variable names.
Is it just me, or others might feel the same ?

John Hunt <huntj>
Thu 06 Feb 2014 10:14:36 PM UTC, comment #11:

I meant that we should only change the user interface of pararrayfun, although the code of the file parcellfun.m might also need to be patched. I did not yet study the patch code, it might be possible that the above is not feasible; then we have a problem.

What regards passing 'as is', you won't convince me. This is not so much a matter of code complication, but rather of maintaining a standard user interface for this standard problem. Passing of unchanged additional arguments can be necessary at many places (optimization, ODE integration) and none of these functions provides passing additional arguments itself nowadays (or shouldn't, there might be relics), since the standard user interface for this problem is using anonymous functions. Also, if the arguments to be passed 'as is' are, e.g., 'a', 'b', and 'c', then writing

does not seem less clean to me than

Olaf Till <i7tiol>
Thu 06 Feb 2014 09:08:28 PM UTC, comment #10:

Seems you perfectly got it.

Just two points to be stressed:

I had to patch parcellfun because it is used by pararrayfun,
and no way around seemed available. Please believe that I thought twice before dwelling into that beast (although remarkably written) !

'As is' passing adds almost no complexity (just a simple condition IIRC).
Thus, given the cleanliness on the user side, I would rather not get rid of it. Please let me one more chance to advocate for it, if needed. I'll try to find a more convincing worked example.

Here is a cleaner patch.

(file #30472)

John Hunt <huntj>
Thu 06 Feb 2014 06:38:13 PM UTC, comment #9:

Only the principle is important now, it is clear that there can be intricate cases.

The advantage in patching pararrayfun seems to be for functions which can vectorize operations between arrays in which one of the arrays has additional dimensions compared to the other array. (This vectorization should require broadcasting of operators or the usage of repmat() for one of the arrays.) This vectorization capability is still effective in parallelization with the patch.

Instead, the same could be achieved by using parcellfun instead of pararrayfun, pretreating the arguments with mat2cell() and concatenating the results with cat(dim, result{:}). Admittedly, the use of mat2cell() can be awkward. Also, pararrayfun already supports vectorized functions (without the above problem with additional dimensions) although the same effect could be achieved for that with mat2cell() and cat(), too.

While there can be a benefit for pararrayfun, I see no decisive benefit for parcellfun, because no vectorization is enabled by the patch. I'd think the patch would only unnecessarily complicate parcellfun.

In any case, we won't support passing of arguments 'as is' in parcellfun/pararrayfun. This is not necessary and is usually done with anonymous functions.

Could you comment if I understood the above issues right and if you could agree with only patching pararrayfun and not supporting passing arguments 'as is'? If we have clearified everything here I would like to take the issue to the maintainers list for opinions on whether it is worth to make the change. It is necessary to be careful, since parcellfun is a prominent function and its code is fairly well organized, so that it's a challenge to add to it with the same code quality.

Olaf Till <i7tiol>
Thu 06 Feb 2014 01:58:55 PM UTC, comment #8:

"IdxDimensions", [0, 0, 0, 0, 1, 1, 1, 0],
"CatDimensions", [4 4 3]
so the indexing is along "n4".
Only e, f and g are indexed (they are column vectors with n4 elements).
All other are passed to and used by the function "as is"
(indicated by 0 in IdxDimensions)

I just realized that indexing is quite simple in this case.
This shows mainly the benefits of CatDimensions.
I also routinely use more intricate ones (indexing multidimensional arrays, not simple vectors). More on that later if you wish.

John Hunt <huntj>
Thu 06 Feb 2014 11:00:54 AM UTC, comment #7:

Could you help me understanding this by indicating whether 'a' and 'c' are rows or columns and the values of 'IdxDimensions' and 'CatDimensions', please.

Olaf Till <i7tiol>
Thu 06 Feb 2014 10:09:16 AM UTC, comment #6:

I see. This was just a test case. In the real case, f has several inputs with different dimensions, for instance
[u, v, w] = f(a, b, c, d, e, f, g, h)
with a is a cell array with n1 dims,
b is (n2, n1) array
c is n3 vector
d is a scalar or vector
e, f, g are (n4 elements vectors or arrays)
h is a structure
and the outputs
u, v are [3, n3, n2, n4] arrays (n1 does not appear here)
w is a [n3, n2, n4] array

There no permutations, and actually the patch allows to pass the arguments straight to the function where they are used in the most efficient/natural way.

John Hunt <huntj>
Thu 06 Feb 2014 09:39:55 AM UTC, comment #5:

I can't see a user advantage of enough generality from your example. Why should a user function which performs element-wise computations in arrays permute its inputs or outputs? Should not the inputs and outputs be maintained in already the correctly permuted form? This would be faster, too.

Olaf Till <i7tiol>
Wed 05 Feb 2014 08:43:18 PM UTC, comment #4:

Here is an usage example, taken from the tests :

The problem with preshaping is that for multidimensional arrays, I found it quite difficult to handle while keeping readable code, hence the patch.

With this patch, you can use exactly the same function and the same inputs with or without pararrayfun.
Just add pararrayfun, IdxDimensions and CatDimensions if needed, and that's it, it runs on multiple cores !
The end user code is very clean.

I'll look for some benchmarks. IIRC, for my big calculations the overhead was negligible. I have made none for small ones.
But anyway no regression should occur since the "default indexing" uses basically the same code as before.

Looking into the code, there are some commented portions I could remove, and a small "fixme" I'll have more time to solve now.

I think that the cleanliness of the user code is worth the slight complication. Please let me know if I should try better to advocate ?

The patch in the current state is working fine, but maybe not clean enough for others to dwell into it. I'm open to polish it before, and try to make a mercurial changeset if you wish.

John Hunt <huntj>
Tue 04 Feb 2014 09:55:48 PM UTC, comment #3:

Since your patch is long, I would like to put off studying its code until I'm convinced that the additional functionality could have enough general usefulness to be worth the complication of the currently very clearcut code of parcellfun/pararrayfun.

pararrayfun already provides calling of vectorized user functions with chunks of the array arguments. What different functionality is provided by your patch?

You write, among others, that some arguments can be passed 'as is'. This should also be achievable with anonymous functions.

Also you write that the dimensions along which to operate can be selected and the outputs can be concatenated along selectable dimensions. This seems of too limited general usefulness to me to be included into parcellfun/pararrayfun. If someone has such special needs, the arguments of parcellfun can be pre-shaped accordingly.

Maybe I've missed the point, if so, please tell me.

Olaf Till <i7tiol>
Mon 03 Feb 2014 08:50:12 PM UTC, comment #2:

Hi,

Could you provide an usage example and some benchmarks?

It would also be great if you could produce a mercurial
changeset for the "parallel" repository instead of a patch.

c.

Carlo de Falco <cdf>
Project Member
Sun 02 Feb 2014 07:45:51 PM UTC, comment #1:

This patch is against general-1.3.2

John Hunt <huntj>
Sun 02 Feb 2014 07:04:28 PM UTC, original submission:

This patch adds the possibility to efficiently use parcellfun or pararrayfun for vectorized functions.
The vector is split in parts, and each job gets a part.

It can work on multiple arguments and multiple outputs.
The dimensions along which to operate can be selected argument by argument. (and some arguments can be passed 'as is', i.e. not splitted).

The outputs can be concatenated along selectable dimensions.

It should work seamlessly for previous programs
(backward compatible).
Lots of tests have been added.

There are talks on the maintainers mailing list about moving theses functions, so I thought better to provide it now, as is.
I have been using it for few months, so it is tested,
but the documentation has to be extended (next weeks I hope).

John Hunt <huntj>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #30472:  parfuns_14206aa.patch added by huntj (21kB - text/x-patch - better patch (some cleanup))
file #30441:  parfuns_14202aa.patch added by huntj (23kB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by i7tiol (Posted a comment)
  • -unavailable- added by cdf (Posted a comment)
  • -unavailable- added by huntj (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 2 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 06 Feb 2014 09:08:28 PM UTChuntjAttached File-=>Added parfuns_14206aa.patch, #30472
    Sun 02 Feb 2014 07:04:28 PM UTChuntjAttached File-=>Added parfuns_14202aa.patch, #30441

    Back to the top


    Powered by Savane 3.1-cleanup