Bugfix --- stride needs to have "value" attribute #488

gopsub · 2025-04-08T20:18:43Z

PR Summary

PR Checklist

Adds a test for any bugs fixed. Adds tests for new features.
Format your changes by using the make format command after configuring with cmake.
Document any new features, update documentation for changes made.
Make sure the copyright notice on any files you modified is up to date.
After creating a pull request, note it in the CHANGELOG.md file.
LANL employees: make sure tests pass both on the github CI and on the Darwin CI

If preparing for a new release, in addition please check the following:

Update the version in cmake.
Move the changes in the CHANGELOG.md file under a new header for the new release, and reset the categories.
Ensure that any when='@main' dependencies are updated to the release version in the package.py

…sure is exposed

jhp-lanl · 2025-04-08T21:18:35Z

CI is failing with the following:

 Error: Variable ‘stride’ at (1) cannot have both the OPTIONAL and the VALUE attribute because procedure ‘get_sg_bulkmodulusfromdensityinternalenergy’ is BIND(C)

gopsub · 2025-04-08T21:24:10Z

CI is failing with the following:

 Error: Variable ‘stride’ at (1) cannot have both the OPTIONAL and the VALUE attribute because procedure ‘get_sg_bulkmodulusfromdensityinternalenergy’ is BIND(C)

It builds on my end, and correctly passes on the value of stride. Without the "VALUE" attribute, the reference is passed (not desirable). What's the solution?

jhp-lanl · 2025-04-09T00:17:54Z

CI is failing with the following:
 Error: Variable ‘stride’ at (1) cannot have both the OPTIONAL and the VALUE attribute because procedure ‘get_sg_bulkmodulusfromdensityinternalenergy’ is BIND(C)
It builds on my end, and correctly passes on the value of stride. Without the "VALUE" attribute, the reference is passed (not desirable). What's the solution?

Are you compiling with Intel? It seems to play a bit fast and loose with the Fortran standard.

I think the fundamental issue is that in C, an optional variable is given a default value. In that sense, it's never not declared, whereas in Fortran the OPTIONAL keyword says that the variable may not ever have a value if it's not provided as an input argument. From the C perspective, there's no logical way to resolve this.

It's possible there's an easier fix, but my suggestion would be to move the default value from the C side to the Fortran wrapper side. So in get_sg_PressureFromDensityInternalEnergy_f() use the check on whether stride is present or not to set a default value of -1 if it's not present. Then the C function just always takes a stride.

jhp-lanl · 2025-04-09T00:22:32Z

Another possibility is to define two C functions with different names, one to accept a stride (and lambda_data) and one that doesn't. Then create two C-interoperable routines in Fortran and then pick which you call. On the C side, this would split the if(stride... statement so that it's just two routines with different behavior. The branching then occurs only on the Fortran side in which function is called.

gopsub · 2025-04-09T12:30:00Z

How's this? Sorry it's a bit cluttered because I worked off the gss/entropy branch to get all 3 functions that need lambdas

Yurlungur

Good catch! I'm happy with this version if @jhp-lanl is.

jhp-lanl

I think this will work

jhp-lanl · 2025-04-09T16:24:54Z

Let me at least start the LANL tests to make sure this all works

jhp-lanl · 2025-04-09T16:44:15Z

It looks like the LANL CI is having an issue with the different definitions:

ftn-855 ftn: ERROR SINGULARITY_EOS, File = ../../../../usr/WS2/jeffreyp/xcap-gitlab-ci/jeffreyp/builds/dDssG5uCy/002/xcap/oss/singularity-eos/singularity-eos/eos/singularity_eos.f90, Line = 27, Column = 8 
  The compiler has detected errors in module "SINGULARITY_EOS".  No module information file will be created for this module.
ftn-1295 ftn: ERROR GET_SG_ENTROPYFROMDENSITYINTERNALENERGY_LAMBDA, File = ../../../../usr/WS2/jeffreyp/xcap-gitlab-ci/jeffreyp/builds/dDssG5uCy/002/xcap/oss/singularity-eos/singularity-eos/eos/singularity_eos.f90, Line = 272, Column = 8 
  "get_sg_EntropyFromDensityInternalEnergy" is defined or referenced at line 260 (/usr/workspace/jeffreyp/xcap-gitlab-ci/jeffreyp/builds/dDssG5uCy/002/xcap/oss/singularity-eos/singularity-eos/eos/singularity_eos.f90) and here.  The number of arguments do not match.  Expected 6, but found 8.
ftn-1295 ftn: ERROR GET_SG_PRESSUREFROMDENSITYINTERNALENERGY_LAMBDA, File = ../../../../usr/WS2/jeffreyp/xcap-gitlab-ci/jeffreyp/builds/dDssG5uCy/002/xcap/oss/singularity-eos/singularity-eos/eos/singularity_eos.f90, Line = 298, Column = 8 
  "get_sg_PressureFromDensityInternalEnergy" is defined or referenced at line 286 (/usr/workspace/jeffreyp/xcap-gitlab-ci/jeffreyp/builds/dDssG5uCy/002/xcap/oss/singularity-eos/singularity-eos/eos/singularity_eos.f90) and here.  The number of arguments do not match.  Expected 6, but found 8.
ftn-1295 ftn: ERROR GET_SG_BULKMODULUSFROMDENSITYINTERNALENERGY_LAMBDA, File = ../../../../usr/WS2/jeffreyp/xcap-gitlab-ci/jeffreyp/builds/dDssG5uCy/002/xcap/oss/singularity-eos/singularity-eos/eos/singularity_eos.f90, Line = 335, Column = 8 
  "get_sg_BulkModulusFromDensityInternalEnergy" is defined or referenced at line 323 (/usr/workspace/jeffreyp/xcap-gitlab-ci/jeffreyp/builds/dDssG5uCy/002/xcap/oss/singularity-eos/singularity-eos/eos/singularity_eos.f90) and here.  The number of arguments do not match.  Expected 6, but found 8.

jhp-lanl · 2025-04-09T16:51:07Z

This might be a bug in the Fortran compiler on the MI300 and MI250 machines.

I wonder if it's getting confused about the C vs Fortran definitions of the function. You might change get_sg_PressureFromDensityInternalEnergy to get_sg_PressureFromDensityInternalEnergy_no_lambda on the Fortran side and we can see if that works.

jhp-lanl

Once this passes on the MI250/MI300 machines I think this is good to go

jhp-lanl · 2025-04-09T17:26:37Z

Testing on the LANL CI now

jhp-lanl · 2025-04-09T17:53:31Z

Same issue 😖 @gopsub . I think the C interoperable function needs to be passed all 8 arguments all the time.

I would try one of two solutions:

Rather than using an if statement in the C interoperable function to select the behavior, you can just have two versions of the function that do the different behaviors. Then on the Fortran side, you can bind an interoperable function to each version and then use the logic in the wrapper function to determine which one you call depending on whether the optional arguments are provided or not. You'd need to add an additional C function, but the advantage of this approach would be that the logic would be cleanly handled on the Fortran side. The disadvantage would be that the interface is more complicated.
Keep one version of the C function and make the optional arguments mandatory. Then you can use the Fortran wrapper logic to select the values passed to the function and handle those values the exact same way you already do. You can delete one of the Fortran interoperable functions with this approach. The advantage of this approach is that you have minimal interface functions, but the logic is duplicated on the C and Fortran sides.

Yurlungur · 2025-04-09T17:57:59Z

I think option #2 is probably slightly preferable IMO, as it reduces code duplication, and this function essentially only exists for the fortran API anyway.

gopsub · 2025-04-09T18:18:38Z

Took a crack at option 2

jhp-lanl · 2025-04-09T20:23:47Z

Running the CI on LANL

jhp-lanl · 2025-04-09T21:46:35Z

Tests pass!

gopsub and others added 15 commits March 26, 2025 16:27

Exposed entropy in fortran interface, in a manner similar to how pres…

c0a8039

…sure is exposed

Fixed formatting, typo

046f814

Formatting

8261ebd

Merge branch 'main' into gss/entropy

c6f9400

Fix eospac_opts_values handling

459ef58

Update CHANGELOG

838b955

Move scratch_size and max_scratch_size into base class

6213e04

Make scratch_size and max_scratch_size const

ef3810f

Update copyright

939e64f

Clang format

a5e6053

Update the changelog

2e795cc

Whoops... forgot eos_powermg

6899e2c

Bugfix --- stride needs to have "value" attribute

840eca4

Merge branch 'gss/fortran/stride' into gss/entropy

815abfc

Added value attribute to entropy interface

65cfd0d

Added _lambda versions of functions to fortran interface

d70e775

Yurlungur approved these changes Apr 9, 2025

View reviewed changes

jhp-lanl approved these changes Apr 9, 2025

View reviewed changes

jhp-lanl requested changes Apr 9, 2025

View reviewed changes

Added _nolambda

64164a7

Removed _nolambda functions

4494ab8

Removed _lambda suffix

4b873b4

jhp-lanl approved these changes Apr 9, 2025

View reviewed changes

jhp-lanl merged commit 1823d15 into main Apr 9, 2025
18 checks passed

jhp-lanl deleted the gss/fortran/stride branch April 9, 2025 21:47

Bugfix --- stride needs to have "value" attribute #488

Bugfix --- stride needs to have "value" attribute #488

Uh oh!

Conversation

gopsub commented Apr 8, 2025

PR Summary

PR Checklist

Uh oh!

jhp-lanl commented Apr 8, 2025

Uh oh!

gopsub commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhp-lanl commented Apr 9, 2025

Uh oh!

jhp-lanl commented Apr 9, 2025

Uh oh!

gopsub commented Apr 9, 2025

Uh oh!

Yurlungur left a comment

Choose a reason for hiding this comment

Uh oh!

jhp-lanl left a comment

Choose a reason for hiding this comment

Uh oh!

jhp-lanl commented Apr 9, 2025

Uh oh!

jhp-lanl commented Apr 9, 2025

Uh oh!

jhp-lanl commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhp-lanl left a comment

Choose a reason for hiding this comment

Uh oh!

jhp-lanl commented Apr 9, 2025

Uh oh!

jhp-lanl commented Apr 9, 2025

Uh oh!

Yurlungur commented Apr 9, 2025

Uh oh!

gopsub commented Apr 9, 2025

Uh oh!

jhp-lanl commented Apr 9, 2025

Uh oh!

jhp-lanl commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

gopsub commented Apr 8, 2025 •

edited

Loading

jhp-lanl commented Apr 9, 2025 •

edited

Loading