Support 2D Arrays for Fortran #169
Conversation
There was a problem hiding this comment.
Hi @KeithBallard , I'm sorry I didn't get any notification about this pull request! Overall it looks quite good, but the one critical missing piece is testing for the new capability. It looks like the only test I have for %fortran_array_pointer is in Examples/fortran/thinvec, but there are other array mapping tests in Examples/test-suite/fortran/fortran_array_typemap_runme.f90 (using Examples/test-suite/fortran_array_typemap.i) so could you cook something up for the new ARRAY[][] typemap for that as well? If you have a local SWIG build, to test you just need to cd $BUILD/Examples/test-suite/fortran && make fortran_array_typemap.cpptest.
Another concern is that in C/C++, ARRAY[][] is not valid: you can only have single-dimensional arrays ARRAY[] and an array of pointers to pointers ARRAY**, and multi-D arrays where only the largest dimension is unknown ARRAY[][ANY][ANY]. I guess if SWIG can parse [][] it's OK?
My other concern is that it was enough of a hack for me to assume 1-D arrays are contiguous (before the F2008 CONTIGUOUS dummy argument modifier), and it seems even more sketchy with 2D arrays. We might want to add validation code for the typemap-in conversions: passing the second array element of each dimension to make sure the stride and offset work out.
Source/Modules/fortran.cxx
Outdated
| Delete(cdecl); | ||
| String *cdecl2 = SwigType_lstr(ctype, imarg); | ||
| Wrapper_add_localv(cppfunc, imarg, cdecl2, NULL); | ||
| Delete(cdecl2); |
There was a problem hiding this comment.
I think this change is accidental?
There was a problem hiding this comment.
Ah, that was not an accident, but I definitely should have broken that out into a separate pull request. When compiling Swig from source with VS2022 (though I suspect other versions of MSVC will encounter the same issue), cdecl is a keyword and cannot be used as a variable name. I just added a 2 to avoid the conflict, but if you see a logical name that avoids the conflict, it could be improved.
There was a problem hiding this comment.
Aha, I actually encountered this error much later when I was updating the upstream SWIG merge request. It's fixed on the fortran branch and I'm cherry-picking onto master now.
| @@ -167,6 +209,9 @@ SWIGINTERN SwigArrayWrapper SwigArrayWrapper_uninitialized() { | |||
|
|
|||
| %typemap(bindc) CTYPE* = FORTRAN_INTRINSIC_TYPE*; | |||
|
|
|||
| //////////////////////////////////////////////////////////////////////////////////////////////// | |||
| // 1D Arrays | |||
There was a problem hiding this comment.
Minor quibble, but can you use separators consistent with the other code in the SWIG library code?
|
First, I apologize for a very belated response. Over the last month or so, I transitioned from a university position to a scientist at AFRL. I would like to stay engaged with this project and will try to be more responsive in the future. Thank you for the input and direction. I will take a look at your test-suite for the 1D arrays, and I will add some additional tests for these wrappers later this week. You bring up an interesting point. Honestly, I didn't realize ARRAY[][] had to be C/C++ syntax. SWIG did not complain about when I tested it in our code. I am actually using today. Regarding your last point, yeah...I am assuming contiguity of the memory. I don't know a straight forward way to check if that assumption is satisfied, but I honestly see it as something that if documented in the API clearly is up to the developer? What do you think? However...padding is something I worry about for aligned 2D arrays. Perhaps I can check for that in the wrapper. I will think about it some more. |
Congratulations! No worries on the response: I myself didn't see your MR until over a month past its creation.
Yes, SWIG's parser and internal processing aren't very standards-compliant. I also had tried something like
I think you could write a templated (or SWIG "templated" fragment) C++ function template<class T>
bool is_contiguous(int nr, int nc, T* r1c1, T* r1c2, T* r2c1)
{
if (nc > 1 && r1c2 != r1c1 + nr) return false;
if (nr > 1 && r2c1 != r1c1 + nc) return false;
return true;
}and add corresponding logic to call from the fortran side. Of course, we could also wait for F2008's |
* Added macros for 2D arrays in fortranarray.swg
* Added prox struct to hold reference to a 2D array
(pointer, # of rows, and # of cols) in fundamental.swg
* Added fragments and typemaps for 2D array proxy
in fundamental.swg
* Added typemaps for the 2D arrays in typemaps.i
To apply the typemaps to a function like:
void foo(double* x, int x_rows, int x_cols);
Use:
%apply (SWIGTYPE *DATA, size_t ROWS, size_t COLS)
{ (double *x, int x_rows, int x_cols) };
KeithBallard
force-pushed
the
support_2d_arrays
branch
from
38622b3
to
290eccd
Compare
|
Note: I just rebased on master to incorporate the variable name change. |
I like the idea, though I could see the test failing for 2D arrays with 1-row, 1-column, or a single value. I suppose there can be checks for these special cases, but it does add further complexity. A CONTIGUOUS attribute would be the most ideal. It appears supported by my compiler (Intel Fotran 2022), but is SWIG-FORTRAN based tied to the 2003 standard? |
I am developing on Windows, and I could not figure out how to build that test-suite. I ran the SWIG project tests, but that only tested the binary. Do you have any experience building them for Windows? However, I did add to your test to cover the new functionality (though I did not test it myself). |
|
@KeithBallard I haven't developed on Windows since I was a student worker way back in college, so I can't help you out there. Since you're in the scientific computing field I would imagine using a linux environment via WSL would be a good option in general. As it stands, the test is failing, among other things a missing |
No worries. Since just transferring jobs, I have a lot of work ahead of me to set up various dev environments. I haven't set up my Linux environment yet. I bounce between Linux and Windows since our software has to be used on both systems. I will sort out the errors after I set up Linux, though I wish the test suite could be built on Windows. Perhaps I will try to get both working. |
Hi All,
I have been working on a large C++ project implementing new extended finite element methods. I have been using SWIG for quite a few years to bind my C++ code to Python, but I am quite new to this fork, which is awesome by the way. One thing we often require is passing relatively small dense matrices in and out of functions/methods. Your project provides seamless support for 1D arrays, so I took that as an example and implemented the same integration for 2D arrays. I have tested it for our code, but I am unfamiliar with your test framework, so any help on that front would be appreciated. I look forward to your thoughts and suggestions.