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 modules and interfaces for this code. #226

Open
edwardhartnett opened this issue Sep 29, 2020 · 15 comments
Open

Add modules and interfaces for this code. #226

edwardhartnett opened this issue Sep 29, 2020 · 15 comments
Labels
enhancement New feature or request

Comments

@edwardhartnett
Copy link
Contributor

edwardhartnett commented Sep 29, 2020

As suggested by @kgerheiser

Kyle please correct me if I am wrong here...

The problem is that we have a bunch of Fortran functions which actually work for different types (kinds). So the way we cope with this is to build several versions of the library with different names, with the kind in the library name.

Another way to handle this, as Kyle is recommending, is to have a Fortran interface, and then have a function for each kind. The interface would map between the generic name and the specific Fortran function which handles that kind.

This is what we do in the PIO library with this code, for example:

  !>
  !! PIO_initdecomp is an overload interface the models decomposition to pio.
  !! initdecomp_1dof_bin_i8, initdecomp_1dof_nf_i4, initdecomp_2dof_bin_i4,
  !! and initdecomp_2dof_nf_i4 are all depreciated, but supported for backwards
  !! compatibility.
  !<
  interface PIO_initdecomp
     module procedure PIO_initdecomp_dof_i4  ! previous name: initdecomop_1dof_nf_box
     module procedure PIO_initdecomp_dof_i8  ! previous name: initdecomop_1dof_nf_box
     module procedure initdecomp_1dof_nf_i4
     module procedure initdecomp_1dof_nf_i8
     module procedure initdecomp_1dof_bin_i4
     module procedure initdecomp_1dof_bin_i8
     module procedure initdecomp_2dof_nf_i4
     module procedure initdecomp_2dof_nf_i8
     module procedure PIO_initdecomp_bc
  end interface PIO_initdecomp

So to implement Kyle's suggestion, we would add an interface of the existing function name, add new functions for each of the supported kinds, and use the interface statement. Then we would compile the library one time, with one name.

This is a backward compatible change, because calling fortran code does not have to change. (But their build system changes, because we would be changing the library name.)

Do I understand you correctly Kyle? I see other libraries with similar-looking build instructions. Other than the sp library, which ones do we build different libraries for different fortran kinds?

@kgerheiser
Copy link
Contributor

kgerheiser commented Sep 30, 2020

Almost all of the libraries are like this, except a few.

Now, Fortran doesn't have generics so the options are to either copy and paste the code multiple times or to use include files and define the kind.

I went with the latter approach.

What do you think of this? I made a Python script to automate the work. It takes the subroutine and makes the body of the subroutine an include file and I use the C pre-processor to define the kind. This avoids duplicating the code.

As an example:

module spanaly_mod
use iso_fortran_env, only: real32, real64, int32, int64

interface SPANALY 
    module procedure SPANALY_4 
    module procedure SPANALY_d 
    module procedure SPANALY_8 
end interface 

contains 

#define REAL_KIND_T real(real32) 
#define INT_KIND_T integer(int32)
subroutine SPANALY_4(I,M,IM,IX,NC,NCTOP,KM,WGT,CLAT,PLN,PLNTOP,MP, &
    F,SPC,SPCTOP)
#include "SPANALY.F90.inc" 
end subroutine SPANALY_4 
#undef REAL_KIND_T
#undef INT_KIND_T

#define REAL_KIND_T real(real64) 
#define INT_KIND_T integer(int32)
subroutine SPANALY_d(I,M,IM,IX,NC,NCTOP,KM,WGT,CLAT,PLN,PLNTOP,MP, &
    F,SPC,SPCTOP)
#include "SPANALY.F90.inc" 
end subroutine SPANALY_d 
#undef REAL_KIND_T
#undef INT_KIND_T

#define REAL_KIND_T real(real64) 
#define INT_KIND_T integer(int64)
subroutine SPANALY_8(I,M,IM,IX,NC,NCTOP,KM,WGT,CLAT,PLN,PLNTOP,MP, &
    F,SPC,SPCTOP)
#include "SPANALY.F90.inc" 
end subroutine SPANALY_8 
#undef REAL_KIND_T
#undef INT_KIND_T

end module spanaly_mod

@edwardhartnett
Copy link
Contributor Author

Firstly, I think the important thing is the use of the interface so that we don't need multiple include directories and installed libraries. That's very important and we must do it.

Secondly, you have presented a potential idiom for solving this with our code. This is one way to achieve what we need and looks good. I'm not usually in favor of using include files like this, but with Fortran you have to make some compromises. ;-) This is a very clever solution which prevents a lot of repeated code.

Your python script seems useful as a way for you to execute these changes, and then we move on with the new Fortran code. I'm not much in favor of adding python scripts to the code base which then become part of the build. It makes debugging very confusing! But as a programmer's tool, such a script is great. And since it is not becoming part of the repo it doesn't need to be tested, documented, etc.

Can you submit a PR to this repo with these changes? Then we can take a look at what it means in practice...

@edwardhartnett
Copy link
Contributor Author

@GeorgeGayno-NOAA suggests simply using double for all computations. Would that work better?

@kgerheiser
Copy link
Contributor

kgerheiser commented Sep 30, 2020

Perhaps? That would certainly be simpler. I don't actually know whether we need each kind.

Though, I do know that _8 with 64 bit integers is probably not used, and I think that applies to most/all of the libraries.

@kgerheiser
Copy link
Contributor

I looked at several places this library is used in the UFS_UTILS and the weather model and they're using _d (64 bit reals and 32 bit integers). Making it all double precision may be viable.

@edwardhartnett
Copy link
Contributor Author

If we made it all double, would we still want an interface? I believe the answer is yes...

@kgerheiser
Copy link
Contributor

Yes.

Looking a little more though, I see post uses the 32 bit version of various libraries.

@edwardhartnett
Copy link
Contributor Author

OK, so we can see this as a 2-step process.
1 - add the interface, supporting all the kinds currently supported.
2 - if some kinds are not needed, remove them.

@kgerheiser
Copy link
Contributor

My point was that if we only support one kind we can lose the whole include hack and just put them in modules and be done.

@edwardhartnett
Copy link
Contributor Author

@kgerheiser yes I understand and agree. However this will be invisible to the user in any case.

The user will greatly benefit if we can change to using the normal include directory name, and go to one library instead of 3. That's a great simplification for the user and for us to support.

If we can also simplify the code by removing unused kinds, that would be a great follow-on. However, there may be users out there using this code in a way we don't know, so we may not be able to eliminate a kind, even if we don't use it in our packages.

In any case, I think you should proceed on this issue in this library, and set an example and precedent for how this will be done elsewhere in NCEPLIBS...

@kgerheiser
Copy link
Contributor

Sounds good

@edwardhartnett
Copy link
Contributor Author

@kgerheiser can we take a stab at this, this week?

@kgerheiser
Copy link
Contributor

kgerheiser commented Oct 5, 2020

Yes, I'll put together a working prototype.

@edwardhartnett
Copy link
Contributor Author

As part of this we will need tests, at least for some of the functions, if not all of them, to make sure your changes are good and everything still works. So what would be good would be to add some of the tests first, then attempt your change.

@opoplawski
Copy link

Any progress here? Thanks.

@edwardhartnett edwardhartnett added the enhancement New feature or request label Jan 26, 2023
@AlexanderRichert-NOAA AlexanderRichert-NOAA transferred this issue from NOAA-EMC/NCEPLIBS-sp Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants