Skip to content
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

Add a test utest.c for the C interface #67

Merged
merged 6 commits into from
Oct 10, 2024
Merged

Add a test utest.c for the C interface #67

merged 6 commits into from
Oct 10, 2024

Conversation

amontoison
Copy link
Member

@amontoison amontoison commented Sep 10, 2024

#64
@jfowkes @nimgould
I confirm that the Fortran routines that uses string / characters as arguments are not working when called from C.
I finished a test in C that I started a few times ago.

Nick, when you will back from from holidays, use this branch with:

git fetch origin
git checkout c_tests

To easily run the new unit tests:

meson setup builddir -Dtests=true -Dquadruple=true --buildtype=debug
meson test -C builddir

@nimgould
Copy link
Contributor

I had a quick look when I was eating my toast today. You need to associate a unit number with the input file using the CUTEst routine FORTRAN_open_r instead of fopen. Have a look at the example in genc.c in ./src/gsl/gsl_main.c

Back to my breakfast ...

@amontoison
Copy link
Member Author

Thanks Nick!
I fixed it we now have errors on routines with *names*, as expected when called from C.

Call CUTEST_udimen
* n = 5
Call CUTEST_usetup
 *       i      X_l          X          X_u
 *       1 -100000000000000000000.0000       0.0000 100000000000000000000.0000
 *       2 -100000000000000000000.0000       0.0000 100000000000000000000.0000
 *       3 -100000000000000000000.0000       0.0000 100000000000000000000.0000
 *       4 -100000000000000000000.0000       0.0000 100000000000000000000.0000
 *       5 -100000000000000000000.0000       0.0000 100000000000000000000.0000
Call CUTEST_unames
Erreur de segmentation (core dumped)

We can fix it together after your holidays 🏖️

@nimgould
Copy link
Contributor

I corrected this (I hope). C and fortrand strings do not coexist very happily, so I hope that this bodge is sufficient. Really we should be using bind(C) in CUTEst as we do in GALAHAD. But that is for another year (unless this fix doesn't work for you). @jfowkes may have an oppinion on this!

I also added a new tool (cutest_classification) that returns the SIF classification string for the problem. This was a request from Bill Hager (and it forced me to do strings "properly") as he is a 100% C man.

No, my holiday isn't done yest, so any comments (etc) can wait for another 7 days

@amontoison
Copy link
Member Author

amontoison commented Sep 29, 2024

I quickly checked your two commits and I have the following comments:

  • For C routines related to "names", we should probably add _cstring_ in the name and not _cint_. I don't see the relation with integers in the new variants of these routines.
  • In include.h, you defined FSTRING_LEN and FCSTRING_LEN for Fortran. Maybe we should also add the length for C?

Enjoy the last week of your holidays Nick!

@nimgould
Copy link
Contributor

nimgould commented Sep 30, 2024

Cint does not refer to C integers, it refers to those fortran procedures that require extra C
interfaces (that is what Cint is short for). Unfortunate, I realise, but these have been out there for 10 plus years, so we can't change this terminology (plus anything longer would overflow the fortran 31 character restriction). The current Cint procedures deal with the logical <-> bool issue, nothing to do with integers (which are fortunately naturally compatible).

The FSTRING_LEN is simply manipulated within the C programs to that C declarations are FSTRING_LEN + 1. Of course we could do this, but I am not sure there is a big advantage.

@amontoison
Copy link
Member Author

Ok I now understand why you use cint!
I updated some tests but more work is needed.
I will finish that later...

@amontoison
Copy link
Member Author

@nimgould
May I ask you some help to fix utest.c on this branch c_tests?
I don't want to do a new release before that testing the C interface for routines _names and the new routine cutest_classification.

@nimgould
Copy link
Contributor

I will have a look. This may be within my C competence ... or not :)

@nimgould
Copy link
Contributor

nimgould commented Oct 10, 2024

The issue is that C and fortran strings do not operate smoothly together, and to do so we have to use pointers on the C side. So, we need to setup via the rather compilcated

char *pname, *vnames
char **Vnames; /* vnames as an array of strings */

then

    /* set problem, variables and constraints names */
    MALLOC(pname, FSTRING_LEN + 1, char);
    MALLOC(vnames, CUTEst_nvar * ( FSTRING_LEN + 1 ), char); /* For Fortran */
    MALLOC(Vnames, CUTEst_nvar, char *);               /* Array of strings */
    for (i = 0; i < CUTEst_nvar; i++)
        MALLOC(Vnames[i], FSTRING_LEN + 1, char);
        CUTEST_unames_r( &status, &CUTEst_nvar, pname, vnames );

/* Make sure to null-terminate problem name */
    pname[FSTRING_LEN] = '\0';

    printf(" Problem: %-s\n", pname);

   /* Transfer variables and constraint names into arrays of
     * null-terminated strings. Is this optimal??
     */
    for (i = 0; i < CUTEst_nvar; i++)
    {
        cptr = vnames + i * ( FSTRING_LEN + 1 );
        for (j = 0; j < FSTRING_LEN; j++)
        {
            Vnames[i][j] = *cptr; 
            cptr++;
        }
        Vnames[i][FSTRING_LEN] = '\0';
    }
    /* Fortran strings no longer needed */
    FREE(vnames);

   printf(" Variable names:\n");
    for (i = 0; i < CUTEst_nvar; i++)
        printf("  %s\n", Vnames[i]);

    /* Free memory for variable names */
    for (i = 0; i < CUTEst_nvar; i++) FREE(Vnames[i]);
    FREE(Vnames);

For the cutest_classification, it is

    char *classification;
    MALLOC(classification, FCSTRING_LEN + 1, char);
    CUTEST_classification_r( &status, &funit, classification );
    printf(" Classification: %-s\n", classification);

plus probably some more freeing.

Sorry, I tried to do this on your branch, but as usual I had a git catastrophe, so best leave this to you.

Maybe @jfowkes has a cleaner way?

@amontoison amontoison merged commit e5cfa23 into master Oct 10, 2024
29 checks passed
@amontoison amontoison deleted the c_tests branch October 10, 2024 18:25
@amontoison
Copy link
Member Author

Hi @nimgould,

I updated utest.c, but the tests are not working on all platforms; however, they work fine on my laptop. I merged the PR so you can include it in the tests of your "makemaster" build system. I left a few "FIX ME!" comments inside, but it’s still better than having no tests in C.

We should also create a ctest.c based on ctest.F90 one day, but that requires some work and none of us are motivated to do it.

I released version 2.3.0 with your latest modifications.

@nimgould
Copy link
Contributor

Thanks @amontoison I'll process these, and I will add a ctest.c at some stage. I'll also check on all the compilers I have here to see if anything comes up for the utest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants