Like4like #6

mtwest2718 · 2022-01-20T12:25:59Z

This pull request over the work in ticket #1

Generate a .h5 file for every IDL .dac file created
- All 1D and 3D arrays in save_varfiles
- The 1D arrays for grid points save_coordinates
- All the 3D arrays in def_varfiles
Compare the two outputs to make sure they are numerically consistent using compare_output.py in ./idl2hdf5 directory.

The new subroutine that saves individual arrays as hdf5 files is save_param_hdf5 in IO_rot.f90 (lines 339-377).

The one remaining save as .dac subroutine that hasn't been mirrored as a hdf5 call is saving the time coordinates. At some point this needs to be completed but at this stage, I believe #1 is complete.

… to allow the code to compile under gcc10 that has stricter fortran standards implemented (https://gcc.gnu.org/gcc-10/porting_to.html)

…moval of trailing whitespace.

…There ought to be a way to combine them...

…ons)

… in library list

…m_hdf5

…test run_code & settings files.

… type

…piles AND runs.

…ing sim-type comments

…iles sub

…ion cleanup

…code more width compact

…o read

… Also removed commented out old code.

…d the pdb.set_trace() comment.

…ration

…iles in idl2hdf5 dir

omarjamil · 2022-01-20T13:39:55Z

HC_rot.f90

@@ -306,12 +306,12 @@ subroutine BiCGstabMPI(matrix,te,source,count)
    double precision,intent(inout)::te(ix,jx,kx),source(ix,jx,kx)
    double precision residual(ix,jx,kx)
    integer i,j,k,n
-    double precision total_resi,total_source,tmp
+    double precision total_resi(1),total_source(1),tmp


It is worth noting that the changes in this file are related to compiling with newer gfortran compiler and not hdf5 changes.

Was this one of your changes at the beginning because I do not remember making this?

Yes, I made that change.

omarjamil · 2022-01-20T13:43:07Z

IO_rot.f90

+          close(mf_z)
+          close(mf_dz)
+          ! now save as hdf5
+          call save_param_hdf5(z, "z.dac." // tmp_id, 1, (/INT(kx, KIND=8)/))


I thought the idea was to have the type conversion take place in the save_param_hdf5 subroutine and not at call time. That would make the interface and call a bit cleaner.

I would like to make the interface & call cleaner but could not figure out how, particularly since I am using the same subroutine to save both 1D and 3D arrays.

omarjamil

I have a few comments about various changes that seem to inappropriate for the main branch merge. Also, the output *h5 files should not be added to the repo; they might be useful for test/examples directory where you could test your installation, but that is not being done at this stage.

omarjamil · 2022-01-20T13:44:05Z

IO_rot.f90

        call save1param(Nexcite(:,:,:,1),tno//'nexcite1.dac.',1)
        call save1param(Nexcite(:,:,:,2),tno//'nexcite2.dac.',1)
        call save1param(Nexcite(:,:,:,3),tno//'nexcite3.dac.',1)
        call save1param(Nexcite(:,:,:,4),tno//'nexcite4.dac.',1)
        call save1param(Nexcite(:,:,:,5),tno//'nexcite5.dac.',1)
        call save1param(Nexcite(:,:,:,6),tno//'nexcite6.dac.',1)
+        ! analogous save commands for HDF5 files
+        call save_param_hdf5(Nexcite(:,:,:,1), "nexcite1.dac.", 3, (/INT(ix, KIND=8), INT(jx, KIND=8), INT(kx, KIND=8)/))


See comment above about type conversion in the subroutine.

omarjamil · 2022-01-20T13:46:54Z

IO_rot.f90

+  !
+  ! Saving a single variable array (of arbitrary rank) to an hdf5 file
+  !
+  subroutine save_param_hdf5(data, varname, rank, dims)


The type conversion of the dummy arguments should take place here.

I just pushed some changes that moves dims declaration inside save_param_hdf5. Makes the calls a lot cleaner.

If I could figure out a way to get rank & dim-size from the data array inside the subroutine, rank and the explicit dims declarations would be unnecessary.

omarjamil · 2022-01-20T13:49:59Z

Makefile

@@ -25,30 +25,30 @@ MOD_FILES = util_rot.mod globalvar.mod parameters.mod \
 	boundary_rot.mod solver_rot.mod matrix_rot.mod initial_rot.mod  procedures.mod

 #FC = gfortran
-FC = mpif90 -O2
+#FC = mpif90 -O2
+FC = h5pfc


This is breaking change for anyone using the code right now without hdf5 present. h5pfc is wrapper/helper script for compiling with hdf5. This is not ready to go into main branch as it is. At the very least need to have some kind of flag option to not use this if you want to use the current code configuration.

Again, this is just a merge into the main on our local fork, not production.

omarjamil · 2022-01-20T13:53:18Z

libraries.txt

@@ -0,0 +1,16 @@
+# needed & optional software libraries


This may not for the main branch or perhaps some context for the dependency list. At this stage perhaps something for notes rather than added to the code base.

omarjamil · 2022-01-20T13:53:48Z

par_hdf5_example/compile_hdf5p.txt

@@ -0,0 +1,7 @@
+Use the helper script to compile mpi against hdf5


Also for notes and not the main branch.

omarjamil · 2022-01-20T13:54:10Z

par_hdf5_example/hyperslab_by_col.f90

@@ -0,0 +1,133 @@
+!


Not for the main branch commit.

omarjamil · 2022-01-20T13:55:07Z

run_code.sh

@@ -0,0 +1,8 @@
+#!/bin/bash


This platform dependent so needs to be clarified. Perhaps something for an "examples" directory.

omarjamil · 2022-01-20T13:55:34Z

setting.txt

@@ -0,0 +1,101 @@
+Data/:outdir; output directory (This should written at the first line in this file!!)


This should also reside in an examples directory.

mtwest2718 · 2022-01-20T14:30:07Z

To be clear, this Pull Request is just within UniExeterRSE's fork of the PIP code-base. None of dev-test material will be going out to the production repo when we finish up the development. I can write another issue as a reminder to remove all of those scripts, directories and the like before merging into @AstroSnow main repository. Would that be satisfactory @omarjamil?

omarjamil

I appreciate this is all on the fork, but still good to have a gold standard branch, master in this case. This a work-in-progress code review and all of the things being committed will likely create confusion on the master branch down the line e.g. when creating a pull request with Ben's upstream branch would need to remember and know which files to remove and move around (e.g. output files should not be added). If you keep the master branch merges as clean and as working (suitable for regression testing) then that would likely save time in the future. I am happy to review and have this conversation, but not sure this needs to go in the master just yet. Perhaps make this a draft pull request instead.

omarjamil · 2022-01-20T16:13:52Z

IO_rot.f90

+    character(len=40) :: fpath      ! Relative path for output file
+
+    ! Define file path based on rank type
+    if (rank.eq.3) then


It is good to see type casting moved into the subroutine. I guess ix,iy,iz are globals. This something Ben that might know better, but my concern with this approach is whether ix,iy,iz would need to be different when collecting all the data from all the processors? When I was thinking about type casting within the subroutine, I was thinking about doing something like this: h5screate_simple_f(rank, INT(dims, KIND=8), dspace_id, error). That would keep it general, but needs testing as I am not sure it will work.

Good point on regression testing.

Should I close this Pull Request without merging, then?

I might as well change the name of this branch to dev to contrast that with main and not have it be so specifically tied to a single issue.

ix, iy, iz are local variables. You might need to create a global version (ixg?) that would need to be allocated in MPI_rot.f90 (before line 26 which is where ix, iy, iz become local). There is also the set_mpi_pos subroutine that is used to determine the location of each processor so this could be useful when collating the data.

omarjamil · 2022-01-20T17:22:20Z

Having a dev branch is a good idea. You could create a new one and move the most relevant changes onto there. Could be a good opportunity to have atomic commits there. Then have a draft PR running between us for reviews and discussions on the implementations.

omar.jamil and others added 30 commits September 28, 2021 20:51

Added setting.txt and unr_code.sh for running the code; Modifications…

1ab8e17

… to allow the code to compile under gcc10 that has stricter fortran standards implemented (https://gcc.gnu.org/gcc-10/porting_to.html)

Added an example for parallel hdf5

eff267a

Draft subroutine for saving single parameters with HDF5. Also auto-re…

984e651

…moval of trailing whitespace.

Naive copy in an attempt to replace dacdef1d with HDF5 file writing. …

5c397ce

…There ought to be a way to combine them...

attempt to merge the 1D and 3D save_param functions together.

4651355

adding list of software libraries to make code work (plus some additi…

945d77b

…ons)

remove outdated hdf5-fortran-openmpi library and some formating fixes…

6f8a6f2

… in library list

removing open-coarrays-bin from library list

00ddb7b

fixed subroutine naming bug and declaring the data array in save_para…

d4097f7

…m_hdf5

adding additional compiler flags to make hdf5 code work properly

11d8321

use h4pfc helper script for compiling code

47730d2

Merge branch 'run_examples' into like4like: needed in particular the …

efafdd6

…test run_code & settings files.

adding one instance of save_param_hdf5 to IO_rot for viscx

c0bd9b1

remove HSIZE_T from integer designation when defining hdf5 dims

ee69bdf

recommended changes, still same compile error with mismatched Integer…

40b8abb

… type

forgot the declaration of dims values being 8 byte INTs. Code now com…

6bf197c

…piles AND runs.

making sure indentation is consistent in save_varfiles along with add…

934a624

…ing sim-type comments

some more two-space alignments to make the code easier to scan

6999283

include save_param_hdf5 for every instance of save1param in save_varf…

5ae80c5

…iles sub

include saving x, dx coordinates to individual hdf5 files

96fe51d

saving y,dy & z,dz coordinates to HDF5 files along with more indentat…

e920c9d

…ion cleanup

move dims arrays in save_param_hdf5 calls to subsequent line to keep …

5e72ff1

…code more width compact

move fpath definition upwards in save_param_hdf5 to make it clearer t…

e71c64c

…o read

IDL code to convert from IDL to HDF5

710a732

Data files for the tests

97be1be

Test plot for data

1e23c59

script to compare ref data in idl2hdf5 to new output via hdf5 directly

0bfed29

format cleanup for idl2hdf5 converter so its easier to scan

d82044a

new gitignore file so that ./Data/ isn't tracked

a782aa5

explicitly adding the .o, .a, and .mod file types to gitignore

e20c7e2

mtwest2718 and others added 8 commits January 13, 2022 16:11

add save_param_hdf5 calls to def_varfiles for all calls of save1param

88c0194

standardizing indentation in "output" subroutine for easier scanning.…

21b0357

… Also removed commented out old code.

Making comp be a boolean array, not a single boolean value. Also move…

e5b8c43

…d the pdb.set_trace() comment.

add *.out to the gitignore

d309745

delete old commented-out version of def_varfiles

2e3d52c

compact if-then's in def_varfiles and remove unused integer i decla…

5d80ee6

…ration

Saved conservative variables for testing

6918a86

changed comparison script to check against "conservative variables" f…

53e2fd6

…iles in idl2hdf5 dir

mtwest2718 added the enhancement New feature or request label Jan 20, 2022

mtwest2718 requested review from omarjamil and AstroSnow January 20, 2022 12:25

mtwest2718 self-assigned this Jan 20, 2022

omarjamil reviewed Jan 20, 2022

View reviewed changes

omarjamil suggested changes Jan 20, 2022

View reviewed changes

mtwest2718 requested a review from omarjamil January 20, 2022 14:56

mtwest2718 added 2 commits January 20, 2022 15:18

move dims declaration into save_param_hdf5

6eb9783

add .h5 file type to .gitignore

2e2d0ad

omarjamil suggested changes Jan 20, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Like4like #6

Like4like #6

mtwest2718 commented Jan 20, 2022

omarjamil Jan 20, 2022 •

edited

Loading

mtwest2718 Jan 20, 2022

omarjamil Jan 20, 2022

omarjamil Jan 20, 2022

mtwest2718 Jan 20, 2022

omarjamil left a comment

omarjamil Jan 20, 2022

omarjamil Jan 20, 2022

mtwest2718 Jan 20, 2022

mtwest2718 Jan 20, 2022

omarjamil Jan 20, 2022

mtwest2718 Jan 20, 2022

omarjamil Jan 20, 2022

omarjamil Jan 20, 2022

omarjamil Jan 20, 2022

omarjamil Jan 20, 2022

omarjamil Jan 20, 2022

mtwest2718 commented Jan 20, 2022

omarjamil left a comment

omarjamil Jan 20, 2022

mtwest2718 Jan 20, 2022

AstroSnow Jan 21, 2022

omarjamil commented Jan 20, 2022

		@@ -0,0 +1,7 @@
		Use the helper script to compile mpi against hdf5

		@@ -0,0 +1,101 @@
		Data/:outdir; output directory (This should written at the first line in this file!!)

Like4like #6

Are you sure you want to change the base?

Like4like #6

Conversation

mtwest2718 commented Jan 20, 2022

omarjamil Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarjamil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtwest2718 commented Jan 20, 2022

omarjamil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarjamil commented Jan 20, 2022

omarjamil Jan 20, 2022 •

edited

Loading