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 comparison operator for boxarray and distromap. Add hdf5 to dep.py #4173

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

dunhamsj
Copy link
Contributor

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:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • include documentation in the code and/or rst files, if appropriate

@@ -86,4 +86,9 @@ extern "C" {
Box bx(IntVect(lo), IntVect(hi), ba->ixType());
return ba->intersects(bx);
}

bool amrex_fi_boxarray_issame (const BoxArray& baa, const BoxArray& bab)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use const BoxArray* instead of const BoxArray&, because const & is not part of C. We probably should also use int on the C/C++ side and c_int on the Fortran side, because Fortran logical may not be compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, I'll make those changes.

@WeiqunZhang
Copy link
Member

Could we add operator(==)?

@dunhamsj
Copy link
Contributor Author

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

@WeiqunZhang
Copy link
Member

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
Copy link
Contributor Author

dunhamsj commented Sep 30, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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

@WeiqunZhang WeiqunZhang changed the title Add issame functions for boxarray and distromap. Add hdf5 to dep.py Add comparison operator for boxarray and distromap. Add hdf5 to dep.py Sep 30, 2024
@dunhamsj
Copy link
Contributor Author

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
Copy link
Member

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

@dunhamsj
Copy link
Contributor Author

That did it, thanks! I just pushed the changes

@WeiqunZhang
Copy link
Member

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

@dunhamsj
Copy link
Contributor Author

Yes, that works

@WeiqunZhang WeiqunZhang enabled auto-merge (squash) October 1, 2024 19:27
@WeiqunZhang WeiqunZhang merged commit 8349789 into AMReX-Codes:development Oct 1, 2024
59 checks passed
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