AMReX-Codes / amrex

AMReX: Software Framework for Block Structured AMR
https://amrex-codes.github.io/amrex
Other
554 stars 353 forks source link

Add comparison operator for boxarray and distromap. Add hdf5 to dep.py #4173

Closed dunhamsj closed 1 month ago

dunhamsj commented 1 month ago

Summary

Add issame function for boxarray and for distromap for the Fortran interface. Also add "hdf5" to dep.py to suppress warnings.

Additional background

Checklist

The proposed changes:

WeiqunZhang commented 1 month ago

Could we add operator(==)?

dunhamsj commented 1 month ago

Sorry, I don't know what you mean by adding operator(==), but I'm happy to try it.

WeiqunZhang commented 1 month ago

Something like

--- a/Src/F_Interfaces/Base/AMReX_boxarray_mod.F90
+++ b/Src/F_Interfaces/Base/AMReX_boxarray_mod.F90
@@ -9,8 +9,7 @@ module amrex_boxarray_module

   private

-  public :: amrex_boxarray_build, amrex_boxarray_destroy, amrex_print, &
-            amrex_boxarray_issame
+  public :: amrex_boxarray_build, amrex_boxarray_destroy, amrex_print, operator(==)

   type, public :: amrex_boxarray
      logical     :: owner = .false.
@@ -37,6 +36,10 @@ module amrex_boxarray_module
 #endif
   end type amrex_boxarray

+  interface operator(==)
+     module procedure amrex_boxarray_issame
+  end interface operator(==)
+
   interface amrex_boxarray_build
      module procedure amrex_boxarray_build_bx
      module procedure amrex_boxarray_build_bxs
@@ -266,8 +269,8 @@ contains
   end function amrex_boxarray_intersects_box

   pure function amrex_boxarray_issame(baa, bab) result(r)
-    class(amrex_boxarray), intent(in) :: baa
-    class(amrex_boxarray), intent(in) :: bab
+    type(amrex_boxarray), intent(in) :: baa
+    type(amrex_boxarray), intent(in) :: bab
     logical :: r
     r = amrex_fi_boxarray_issame(baa%p, bab%p)
   end function amrex_boxarray_issame

This will allow the users to write baa == bab or baa .eq. bab instead of amrex_boxarray_issame(baa,bab). I also changed from class to type, because the types involved are not polymorphic.

dunhamsj commented 1 month ago

I see, thanks. I made those changes, and now in my fortran code, baa .eq. bab returns 0 if the two boxarrays are equal. I just want to be sure, I should then do something like if( ( baa .eq. bab ) .eq. 0 ) to check if they are equal?

WeiqunZhang commented 1 month ago

I am not sure what you are talking about. The fortran function should still return logical, not 0 or 1. Something like return c_function_that_returns_int(...) .ne. 0. Why don't you push what you have so that I can see what changes you have made?

dunhamsj commented 1 month ago

Ah, okay. I think I misunderstood what you were suggesting. I changed all the functions to return integers. I will push my changes though

WeiqunZhang commented 1 month ago

My suggestion is the bind(c) functions use c_int and Fortran native code logical. It's logically true is a c_int is not 0.

WeiqunZhang commented 1 month ago

The reason is I mentioned previously that Fortran logical may not be compatible with C/C++ bool.

With gfotran,

program main
  logical :: a = .true.
  print*, "sizeof(logical) = ", sizeof(a)
end program main

produces sizeof(a) = 4.

But with g++, the sizeof bool is 1. They are not compatible. We could probably use c_bool in Fortran. But _Bool is a C type, not a C++ type. So the safest way is to use int (i.e., c_int) for C interface code and manually convert to logical in Fortran.

dunhamsj commented 1 month ago

That makes sense, thanks. I'm trying it now using logical for the fortran code, integer for the bind(c) code, and int for the c code. I think it's working, I just want to be sure before I push it

dunhamsj commented 1 month ago

Sorry, I couldn't get it to work right. Before the changes, the function would return true when it should have, but after the changes it returns false. I just pushed my changes

WeiqunZhang commented 1 month ago

Okay, I will try to fix it and ask you to test.

dunhamsj commented 1 month ago

That did it, thanks! I just pushed the changes

WeiqunZhang commented 1 month ago

We should also use .ne. 0 to convert from c_int to logical. Could you test?

dunhamsj commented 1 month ago

Yes, that works