Exawind / amr-wind

AMReX-based structured wind solver
https://exawind.github.io/amr-wind
Other
105 stars 83 forks source link

Require density argument for openfast turbine types #1031

Closed mbkuhn closed 5 months ago

mbkuhn commented 5 months ago

Summary

Require density argument for openfast turbine types to prevent default density being used unintentionally

Pull request type

Additional background

OpenFAST-coupled turbine types have an input argument for the density. This is intended to be the same as the density specified in the OpenFAST turbine definition because this density is used to divide the forces from OpenFAST (amr-wind source terms are in units of force/density, i.e. m/s^2). This is a quick way to address the fact that density can be omitted from the input file and users will unintentionally use a default density of 1.0.

Unit tests and documentation do not really apply to this change, and the reg tests are not coupled with OpenFAST.

Issue Number: #1009

mbkuhn commented 5 months ago

I think that a longer term fix would be to

  1. acquire the density value from the openfast interface directly
  2. check the density value from openfast against the amr-wind density value and provide a warning if the two do not match.

Originally I thought it would be fine to get the turbine density value from the amr-wind density value, but then the user could accidentally have a mismatch between the openfast turbine file AirDens and the amr-wind simulation density, which would go undetected.

tonyinme commented 5 months ago

@mbkuhn , I agree, this is a good temporary fix.

lawrenceccheung commented 5 months ago

hi @mbkuhn, I agree with the short term fix and definitely should try and implement the longer term options too. I am trying to get something in amrwind-frontend to automatically set these things and validate the inputs, but the number of times density appears and the complexity of the OpenFAST inputs doesn't make this easy.

My understanding is that the AirDens input from OpenFAST is completely separated from AMR-Wind, so the threat of a mismatch will always be there, until we have an interface which communicates the two. This will also be an issue for the people working on the anelastic implementation if the density is meant to be variable, so a real long term solution would want to take that into account.

mbkuhn commented 5 months ago

I did some digging into the OpenFAST interface today. AMR-Wind relies on calls that are declared in the FAST_Library from openfast-library/src/FAST_Library.h. This relies heavily on an openfoam module in OpenFAST, which I'm guessing is how SOWFA interacts with OpenFAST. In that module, there is already a way to access the AirDens variable, and there are even comments noting that openfoam needs to normalize by that density. So, it shouldn't be that hard to add a way to access that variable directly through the AMR-Wind interface, but it would require an OpenFAST PR.

Not sure how things will be set up with variable density from the turbine modeling side, but this would address the constant density stuff.

lawrenceccheung commented 5 months ago

Also tagging @psakievich and @ndevelder because we've discussed density and the OpenFAST interface before.

ndevelder commented 5 months ago

I did some digging into the OpenFAST interface today. AMR-Wind relies on calls that are declared in the FAST_Library from openfast-library/src/FAST_Library.h. This relies heavily on an openfoam module in OpenFAST, which I'm guessing is how SOWFA interacts with OpenFAST. In that module, there is already a way to access the AirDens variable, and there are even comments noting that openfoam needs to normalize by that density. So, it shouldn't be that hard to add a way to access that variable directly through the AMR-Wind interface, but it would require an OpenFAST PR.

Not sure how things will be set up with variable density from the turbine modeling side, but this would address the constant density stuff.

When I coded the fast->spinner sampler connection I had to get Raf/Andy to add a function to FAST_Library and they seemed totally fine to add things there. I think I was unaware that FAST_Library was connected to the openfoam module in any significant way but I think the connection to Nalu also uses the "openfoam" module? Let me know if you want a hand with this, and I can revisit what I did.

marchdf commented 5 months ago

Just to be clear with the connection with openfast. AMR-Wind goes through the "openfast-library" module: https://github.com/OpenFAST/openfast/tree/main/modules/openfast-library. So we would have to add the interface there. People probably already know this, but I just wanted it out there just in case ;)

ndevelder commented 5 months ago

Thanks @marchdf! And yeah, just to be clear on the dev process, any function that gets added to FAST_Library can be called via AMR-Wind given the name and arg list passed to "fast_func":

https://github.com/Exawind/amr-wind/blob/2da6dd5aa05a16782970b0f86b8437b8f0caa29b/amr-wind/wind_energy/actuator/turbine/fast/FastIface.cpp#L18

psakievich commented 5 months ago

If we get another function put in, we should be proactive about it and try to make it 1) a struct that gets populated so it is easy to add more stuff to it without breaking everything going forward and 2) fill it full of any of the basic redundant properties that we are pulling from openfast but still putting in our input files like timestep

psakievich commented 5 months ago

Putting this here from an AI chat for reminder. Have not vetted the results:

To pass data from a Fortran module to a C struct, you can follow these steps:

Define the C struct in a header file (e.g., "struct.h") that can be included in both the Fortran and C code.
// struct.h
#ifndef STRUCT_H
#define STRUCT_H

struct MyStruct {
    int data1;
    double data2;
};

#endif
In your Fortran module, declare an interface to the C function that will receive the data and include the "iso_c_binding" module.
! my_module.f90

module my_module
    use iso_c_binding

    interface
        subroutine pass_data_to_c(data) bind(C, name="pass_data_to_c")
            import :: C_INT, C_DOUBLE
            type(c_ptr), value :: data
        end subroutine pass_data_to_c
    end interface

    ! Declare your Fortran module variables

    integer :: fortranData1
    real(kind=c_double) :: fortranData2

contains

    subroutine pass_data_to_c_wrapper()
        use iso_c_binding
        use struct_module

        type(c_ptr) :: c_struct_ptr

        ! Allocate memory for the C struct

        c_struct_ptr = c_loc(c_struct)

        ! Assign values to the C struct

        c_struct%data1 = fortranData1
        c_struct%data2 = fortranData2

        ! Call the C function

        call pass_data_to_c(c_struct_ptr)
    end subroutine pass_data_to_c_wrapper

end module my_module
In your C code, include the "struct.h" header file and define the C function that will receive the data.
// main.c
#include <stdio.h>
#include "struct.h"

void pass_data_to_c(struct MyStruct *data) {
    printf("Received data from Fortran:\n");
    printf("Data 1: %d\n", data->data1);
    printf("Data 2: %f\n", data->data2);
}

int main() {
    // Call the Fortran subroutine that will pass data to the C function
    pass_data_to_c_wrapper();

    return 0;
}
Compile and link your Fortran and C code together. Make sure to link against the appropriate libraries and include the necessary flags.
$ gfortran -c my_module.f90
$ gcc -c main.c
$ gfortran -o my_program my_module.o main.o -l<fortran_library> -l<c_library>
Replace <fortran_library> and <c_library> with the appropriate library names for your Fortran and C compilers, respectively.

Run the compiled program.
$ ./my_program
This approach uses the ISO C Binding feature in Fortran to ensure compatibility between the Fortran and C data types. By passing a pointer to the C struct, you can access and manipulate the data in C.
mbkuhn commented 5 months ago

Mike S. made the point that since this is a fluid property, it would ideally be sent from AMR-Wind to OpenFAST, not the other way around. Makes sense to me, but I'm not sure if that would be more difficult to set up.

psakievich commented 5 months ago

Mike S. made the point that since this is a fluid property, it would ideally be sent from AMR-Wind to OpenFAST, not the other way around. Makes sense to me, but I'm not sure if that would be more difficult to set up.

Definitely agree. Maybe we can make this struct a handshake thing? Have openfast populate it, amr-wind reads it, updates/override values, sends back and then openfast initializes its simulation with those values. If that is feasible... 🤷🏻

ndevelder commented 5 months ago

Mike S. made the point that since this is a fluid property, it would ideally be sent from AMR-Wind to OpenFAST, not the other way around. Makes sense to me, but I'm not sure if that would be more difficult to set up.

Definitely agree. Maybe we can make this struct a handshake thing? Have openfast populate it, amr-wind reads it, updates/override values, sends back and then openfast initializes its simulation with those values. If that is feasible... 🤷🏻

Just my two cents, but isn't this a situation where we don't want either code populating a value in the other code because either value of density could be correct? I'd argue for a research code that we'd want the code to stop and report all the input values of density so that the user can fix whichever one is wrong? I'd rather resubmit the correct simulation than have to re-run something after if ran fully with the wrong density value...or worse, have it run fully and not notice that it was off

psakievich commented 5 months ago

Just my two cents, but isn't this a situation where we don't want either code populating a value in the other code because either value of density could be correct? I'd argue for a research code that we'd want the code to stop and report all the input values of density so that the user can fix whichever one is wrong? I'd rather resubmit the correct simulation than have to re-run something after if ran fully with the wrong density value...

IMHO it mainly comes down to consistency. I have always been of the opinion that in an IDEAL world the CFD code could specify everything it defines (turbine positioning, fluid properties, timestep, etc) and openfast would just use those. In the situation I'm describing the CFD code's inputs would always be what is used, and there would be no fluff parameters where you have to specify something just so it matches the OpenFAST input file. If you specify it in the CFD input deck that is what will be used.

I suppose this could be an issue if you were comparing standalone OpenFAST and had a mismatch density value, but that is a lesser evil to me due to the discrepancy in fidelity and resources.

lawrenceccheung commented 5 months ago

Adding another two cents in here, but I would argue that we would definitely want the ability for AMR-Wind to override OpenFAST's inputs. It's not just AirDens, but things like CompInflow and WakeMod are also inputs which can lead to very misleading results if they're not set correctly. It's getting to the point where there are enough possible conflicts that writing the code to figure out the parameter inconsistencies can be tedious, so if we do that, we might as well have the code automatically set them correctly.

Lawrence

marchdf commented 5 months ago

Because this is fun: I like all this discussion and trying to make things consistent. My one worry having amr-wind override stuff in openfast is that the openfast files become non-truthful. Basically, we can't look at the openfast input files (or share them with openfast devs) and know what was actually run because amr-wind might have messed with those values. I get that this is already the case for some stuff (maybe?) but it's worth keeping in mind I think. So maybe this overwrite needs to be verbose?

ndevelder commented 5 months ago

Re: Lawrence's and Marc's comments, I think we're all fully on board with insuring consistency, but I agree more with Marc on a process level. I think we wade into a reproduceability nightmare if we start allowing for model changes not driven by the individual code's input files. I'd strongly advocate for either a series of consistency checks that stop the sim if triggered, or a consistency report that runs all the checks but doesn't stop the sim.

And if we do decide to start overriding, maybe we put a flag in amr-wind to allow users to choose that...I personally don't think I would ever turn it on, just because I'd find it too confusing. However, I could see certain circumstances (like waiting in really long queues) where I might?

And just for completeness, we could also implement a standalone consistency check utility distributed with the code...it could be nice to have something you run while developing your input files as opposed to at simulation time?

lawrenceccheung commented 5 months ago

I fully agree with @marchdf regarding the verbose option -- we would want to be fully transparent to the user that AMR-Wind is changing the behavior of OpenFAST inputs. However, one of the things that has always troubled me that is that when we start setting up larger wind farms, it's very easy to forget to change one of the parameters in one of the turbine input files, and that could lead to some very difficult to find issues later on. However, making the override behavior be optional could work for everyone.

Right now I'm using the amr-wind-frontend to do the consistency checks of the input files:

$ amrwind_frontend.py --validate INPUTFILE.inp
-- Checking inputs --
[ PASS] max_level:           max_level = 1 >= 0
[ PASS] dt & CFL:            DT and CFL OK
[ PASS] restart dir:         Restart directory /lustre/orion/cfd162/scratch/lcheung/ALCC_Frontier_WindFarm/MedWS_LowTI/precursor6_4kmX2km_5m.bndrydata/chk65000 exists
[ PASS] boundary plane dir:  Restart directory /lustre/orion/cfd162/scratch/lcheung/ALCC_Frontier_WindFarm/MedWS_LowTI/precursor6_4kmX2km_5m.bndrydata/bndry_data exists
[ PASS] Actuator physics:    incflo.physics and ICNS.source_terms OK for Actuators
[ PASS] Actuator FST:T0      [T0_OpenFAST3p4_IEA15MW/IEA-15-240-RWT-Monopile/IEA-15-240-RWT-Monopile.fst] exists
[ PASS] Actuator FST:T0      Actuator density=1.245600, matches incflo.density=1.245600
[ PASS] Actuator FST:T0      CompInflow OK
[ PASS] Actuator FST:T0      [T0_OpenFAST3p4_IEA15MW/IEA-15-240-RWT-Monopile/IEA-15-240-RWT-Monopile_AeroDyn15.dat] exists
[ PASS] Actuator FST:T0      WakeMod=0 OK
[ PASS] Actuator FST:T0      AirDens=1.245600, matches incflo.density=1.245600
[ SKIP] Sampling probes:     Not active or no sampling planes

Results: 
 11 PASS
 1 SKIP
 0 FAIL
 0 WARN

It doesn't cover everything, but it has found some very dumb mistakes on my part before.

Lawrence

mbkuhn commented 5 months ago

We have some great discussion on this PR. When we decide to improve the interface, let's link the discussion here to that PR or issue. For now, we have the density required as an argument and a few checks and warnings regarding the density value.