-
Notifications
You must be signed in to change notification settings - Fork 1
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 MPIFirstDerivative #55
Conversation
In the previous PR #49, we had discussed about some edge cases, I have handled them in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I think this PR looks very good and aligned with our discussion.
There is only one thing left, which I think we need to decide on and handle / make explicit in the doc: currently we do not provide axis
to this operator, meaning that we implicitly assume the derivative is always applied to a given axis (looking at the code and how you write the stencils, this looks to me like it is axis=0, see for example: y_forward = ghosted_x[1:] - ghosted_x[:-1]
. However, when you do the appends you seem to do them along y.axis
(which is x.axis
). I suspect that if x.axis>0
we would not be doing the correct thing... do you agree?
So one option is to raise errors if axis!=0
, whilst the other option is to add axis
, check that self.axis==x.axis
and handle the stencils over any axis. In fact if x.axis=0 but self.axis=1, it won't really make sense to use this operator as the derivative stencils do not go across ranks, and users can just leverage pylops.FirstDerivative together with pylops_mpi.BlockDiag like we have done before :)
def _matvec(self, x: DistributedArray) -> DistributedArray: | ||
# If Partition.BROADCAST, then convert to Partition.SCATTER | ||
if x.partition is Partition.BROADCAST: | ||
x = DistributedArray.to_dist(x=x.local_array, axis=x.axis) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmh if the array is of Partition.BROADCAST
, what does x.axis
really mean? I guess from how DistributedArray is implemented right now, one could choose any axis, but here this would lead to problems... see my general comment
|
||
def _matvec_forward(self, x: DistributedArray) -> DistributedArray: | ||
y = DistributedArray(global_shape=x.global_shape, dtype=self.dtype, axis=x.axis) | ||
y[:] = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really needed... if I look at 138, it seems to me you are setting the entire array, so we may be able to avoid setting it to zero first
Thanks |
Mmh my bad, I had forgotten about that... but I think we need to change it and allow Nd arrays. Let me explain why: imagine you have a 3d array This is why I suggested to have no axis and implicitly compute the derivative over axis=0, but to handle the dimensions. Basically, we need to be able to have something that produces the same result as:
if we do
Makes sense? |
Thanks Understood. |
Great! Sorry, maybe we didn't think so deeply about the implications of having only 1d, but I think having a start with 1d only and now move to nd is still a probably better/easier way to do then if we immediately started with nd... and I feel there is little to be done to make this work (I may be wrong ;) ) |
I think what we have done will greatly help as now we would prioritize calculating the derivative along axis=0. |
Hi @mrava87, I think we need to go little different than the 1-D approach, I think pretty much the code in _matvec and _rmatvec will remain the same. |
Hi @mrava87, I have handled the case for
This is what I came up with, which seemed to be the best and apt approach what do you think? |
Thanks @rohanbabbar04, but I am a bit confused by the approach you took... and a bit worried that we are doing a lot of additional communications that may not be needed (mostly referring to the @redistribute decorator). First of all, I am not really sure I understand what Second, perhaps I am missing something, but let me try to explain what I think is a easier way which does not require any communication other than for the ghost cells within the derivative part:
Let's try to agree on this first before changing the code further :D |
Ok, perhaps I get the point... are you trying to handle a case where the operator and x are not agreeing on how the array is distributed? I would say this should be something users have to ensure, and our codes should only check and raise an error otherwise... my worry is that if we silently do this we will end up with unefficients codes as users do not bother about aligning things and internally we do a lot more communications than needed to fix laziness from the user side... or maybe I don't get what the code is trying to handle 😕 |
@mrava87 Yup you are right... |
Sorry I now see that the communication can be really ugly at times, so we should ask the users to reshape their arrays so that it doesn't affect other things... |
So @mrava87 , As you mentioned, now I have raised an error if the |
Right, I see the point about using dims or shape for Nd or 1D arrays, which also when they are of same size, can be distributed differently… but, would this case ever happen. Here you say that you have an array of size Let's however assume that we have this case with nproc=3:
this will be again a problem... Having said that, I am myself unsure right now what is best, just a few thought:
The only way I can think of that strikes a balance between performance and user friendliness is to allow users to choose the splitting of the array, by adding a parameter to DistributedArray. We can default it to None and leave the current behavior as default, and let users choose it only in scenarios where this is useful. What do you think? |
For the other approach, one case is if the dims[0] is completely divisible by the number of processes, the first derivative can calculate..... |
Also for my approach, it expensive only for the cases where there is difference in local shapes but no communication takes place when they are aligned(it is I will say more user convenient)... |
Other that this, I think this new version looks good and almost ready to go :D |
In real life I think this isn't always possible... what if Also, take a look at my last sentence, which I just added in the previous comment. Perhaps having this option + your method will be the best way to go. So lazy users will not need to think but incur in some extra communication cost (this could be also during quick development), whilst users that care about real performance will choose the split of the array in a manner that is consistent with that of the Operator... |
Can you explain it a bit more? |
That's correct. So far the local_shape is automatically detected, which may be what we want to do in most scenarios, but in few cases like this one we will have more freedom to choose a different subdivision between ranks such that things align with an operator acting on one axis of a reshaped view of the total array :) I think this should be doable without modifying DistributedArray that much, isn't it? And, for |
So if I am right we will just ask them the local shapes |
Indeed, with default local_shape=None.. and build it from the global_shape as we do already |
Okay... to summarize everything
|
Yeah changes shouldn't be much.. |
Hi @mrava87, like we discussed this PR contains and solves 2 problems:
I will add the |
Perfect, agree! |
@mrava87 |
Sure :) |
Taking into consideration #49
MPIFirstDerivative