hypre
hypre copied to clipboard
hypre-space
BigJ realloc
I am not sure if this makes sense. The logic is if BigJ exists in CSR, realloc it and ignore J; otherwise realloc J. Apparently, reallocating BigJ is useful for some users. See #622
It's hard for me to sign off on this without a better understanding of how J and BigJ are handled internally. This routine appears to be both an "alloc" and a "realloc" for Data and J (note that I and new_num_rows don't play a roll here at all), but for BigJ only "realloc" is supported.
I agreed. The whole J and BigJ things are complicated. To respond your comments partially, since the BigJ is put inside if (hypre_CSRMatrixBigJ(matrix)), so doing alloc doesn't make sense; only realloc does instead. But on the other hand, we cannot treat BigJ the same as J since we (probably) don't want to alloc BigJ if not necessary (i.e., when it's NULL. In most cases, it's not used).
Ruipeng and I were just chatting about this... The question we should answer first is, "are J and BigJ mutually exclusive data items?" If that is the case, then we should add a boolean to the matrix structure indicating which of them to use. That would clear up logic questions that come up in the future. @ulrikeyang, is that the intention here with these two arrays? Others please feel free to chime in as well.
Yes. BigJ is needed for the extended matrices
Sent from my iPhone
On May 24, 2022, at 4:43 PM, Rob Falgout @.***> wrote:
Ruipeng and I were just chatting about this... The question we should answer first is, "are J and BigJ mutually exclusive data items?" If that is the case, then we should add a boolean to the matrix structure indicating which of them to use. That would clear up logic questions that come up in the future. @ulrikeyanghttps://urldefense.us/v3/__https://github.com/ulrikeyang__;!!G2kpM7uM-TzIFchu!mbiizfA3sDOUO2Ib65tAnqfD4PKUtISofUwgQ9mEzsRWkM5HgdJjppySFmNgDm9a$, is that the intention here with these two arrays? Others please feel free to chime in as well.
— Reply to this email directly, view it on GitHubhttps://urldefense.us/v3/__https://github.com/hypre-space/hypre/pull/623*issuecomment-1136536542__;Iw!!G2kpM7uM-TzIFchu!mbiizfA3sDOUO2Ib65tAnqfD4PKUtISofUwgQ9mEzsRWkM5HgdJjppySFtTQWUBd$, or unsubscribehttps://urldefense.us/v3/__https://github.com/notifications/unsubscribe-auth/AD4NLLNPDPP22ACWTV7VZMDVLVSSBANCNFSM5TZJA6BA__;!!G2kpM7uM-TzIFchu!mbiizfA3sDOUO2Ib65tAnqfD4PKUtISofUwgQ9mEzsRWkM5HgdJjppySFhqrN3mn$. You are receiving this because you were mentioned.Message ID: @.***>
Yes. BigJ is needed for the extended matrices
So, I guess the answer to Rob's question is "yes" (?).
Here is my take on this: Conceptually and originally a hypre_CSRMatrix is a local matrix, and therefore would just need the J data, i.e. 32 bit integers. This of course assumes that this would never get larger than 2 billions. This was always reasonable when we had smaller processors with restricted memory. Even for the Offd matrix this worked out fine, since the column indices were based on the compressed data map. This of course will break down if we end up with much bigger processors (whatever is in an MPI task. I introduced BigJ mostly to deal with the extended matrices, something temporarily used at the developer level during AMG setup to generate interpolation and coarse grid operators or matrix matrix products. So, use of this is somewhat limited. Now, the question is: Do we expect to need this more often even outside the current usage? Of course, we could add the parameter Rob suggests, but it will require a lot of changes throughout the code. If we think that the use of BigJ could happen quite often, this could make sense. If we don't want to add it, Ruipeng's suggested change to this routine is good and actually would fix a potential bug. So, we clearly should do this for now. I will approve this PR for that reason. We can still discuss the other change Rob suggests.
Here is my take on this: Conceptually and originally a hypre_CSRMatrix is a local matrix, and therefore would just need the J data, i.e. 32 bit integers. This of course assumes that this would never get larger than 2 billions. This was always reasonable when we had smaller processors with restricted memory. Even for the Offd matrix this worked out fine, since the column indices were based on the compressed data map. This of course will break down if we end up with much bigger processors (whatever is in an MPI task. I introduced BigJ mostly to deal with the extended matrices, something temporarily used at the developer level during AMG setup to generate interpolation and coarse grid operators or matrix matrix products. So, use of this is somewhat limited. Now, the question is: Do we expect to need this more often even outside the current usage? Of course, we could add the parameter Rob suggests, but it will require a lot of changes throughout the code. If we think that the use of BigJ could happen quite often, this could make sense. If we don't want to add it, Ruipeng's suggested change to this routine is good and actually would fix a potential bug. So, we clearly should do this for now. I will approve this PR for that reason. We can still discuss the other change Rob suggests.
Thanks to @ulrikeyang for clearing it up. Good point on the offd matrix's J. I agreed that what we have now (even without this change I guess) would work just fine in the context of BoomerAMG as people normally use it. On the other hand, having the change @rfalgout suggested can make the code more robust in general, especially for where hypre is used as a "sparse linear algebra" library (such as in MFEM) as reported in the related issue https://github.com/hypre-space/hypre/issues/143 to have a more consistent way to deal with BigJ. I guess it's also the case for changing the realloc here, since I don't think we do relloc on BigJ matrices in BoomerAMG anywhere (?).
I am not sure how much code will be impacted if we implement the change. But I don't mind giving a quick try to have some ideas. Please let me know what you think. Thanks!
Personally I think, we should just approve your current PR, since this could fix a potential bug. Assume we need reallocation for BigJ in a matrix matrix multiplication: it doesn't happen with the current routine. Adding this parameter requires more discussion about the need of BigInt in that data structure. If we really decide to include more stuff regarding BigInt, there could be more questions. Do we maybe need a BigI, etc?
Adding this parameter requires more discussion about the need of BigInt in that data structure. If we really decide to include more stuff regarding BigInt, there could be more questions. Do we maybe need a BigI, etc?
I think this can wait, too. I added a discussion item to our meeting agenda. Thanks!
Yes that was the reason. All the A-ext matrices which deal will external info require those BigJ indices, but generally for local matrices J is fine.
Sent from my iPhone
On May 24, 2022, at 4:43 PM, Rob Falgout @.***> wrote:
Ruipeng and I were just chatting about this... The question we should answer first is, "are J and BigJ mutually exclusive data items?" If that is the case, then we should add a boolean to the matrix structure indicating which of them to use. That would clear up logic questions that come up in the future. @ulrikeyanghttps://urldefense.us/v3/__https://github.com/ulrikeyang__;!!G2kpM7uM-TzIFchu!mbiizfA3sDOUO2Ib65tAnqfD4PKUtISofUwgQ9mEzsRWkM5HgdJjppySFmNgDm9a$, is that the intention here with these two arrays? Others please feel free to chime in as well.
— Reply to this email directly, view it on GitHubhttps://urldefense.us/v3/__https://github.com/hypre-space/hypre/pull/623*issuecomment-1136536542__;Iw!!G2kpM7uM-TzIFchu!mbiizfA3sDOUO2Ib65tAnqfD4PKUtISofUwgQ9mEzsRWkM5HgdJjppySFtTQWUBd$, or unsubscribehttps://urldefense.us/v3/__https://github.com/notifications/unsubscribe-auth/AD4NLLNPDPP22ACWTV7VZMDVLVSSBANCNFSM5TZJA6BA__;!!G2kpM7uM-TzIFchu!mbiizfA3sDOUO2Ib65tAnqfD4PKUtISofUwgQ9mEzsRWkM5HgdJjppySFhqrN3mn$. You are receiving this because you were mentioned.Message ID: @.***>