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

(gr) (hdf5) fixed compile errors when GR=yes and HDF5=yes #58

Merged
merged 8 commits into from
Sep 4, 2020

Conversation

dliptai
Copy link
Collaborator

@dliptai dliptai commented Aug 24, 2020

No description provided.

Copy link
Owner

@danieljprice danieljprice left a comment

Choose a reason for hiding this comment

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

I'm really trying to avoid ifdefs all over the place, would be nicer to use if (gr) rather than #ifdef GR, as it allows for if(gr) to become a runtime option eventually

@dliptai
Copy link
Collaborator Author

dliptai commented Aug 25, 2020

I agree. The biggest roadblock to this is that GR compiles a different externalforces module, and it also does not compile any of extern_*.f90

@dliptai
Copy link
Collaborator Author

dliptai commented Aug 25, 2020

I decided to include all extern_*.f90 in SRCPOTS when compiling with GR=yes. I don't think it should cause any problems, and hopefully it should make removing other #ifdef GRs easier

@danieljprice
Copy link
Owner

thanks for removing the ifdefs (#55), however there is now a conflict with the radiation branch just merged. If you could fix this would be helpful.

@dliptai
Copy link
Collaborator Author

dliptai commented Aug 28, 2020

Conflict resolved.
Updated the hdf5 readwrite to use eos_vars for writing temperature and pressure to dump.

For consistency between the two formats:

  • removed VrelVf and dustgasprop from small dumps
  • tstop array is no longer read from dump

@dmentipl
Copy link
Contributor

@dliptai: Thanks very much for fixing this. It all looks good to me.

@danieljprice: Is it possible for this to be merged?

The MPI test failure is in radiation, not the HDF5 read/write dumps, as far as I can tell.

--> SKIPPING TEST OF RADIATION DERIVS (need -DPERIODIC)

 ---------------- particles set on  32 x   8 x   8 uniform closepacked lattice --------------
  x:  -0.500    ->  0.500       y:  -0.100    ->  0.100       z:  -0.100    ->  0.100     
 dx:   3.125E-02               dy:   2.706E-02               dz:   2.552E-02
                            dy/dx:   0.8660               dz/dx:   0.8165
 enforcing periodicity: nx, ny, nz =     32 x     8 x     6,  np =     1536
 dx:   3.125E-02               dy:   2.500E-02               dz:   3.333E-02
                            dy/dx:   0.8000               dz/dx:   1.0667
 -----------------------------------------------------------------------------------------

 checking   grad{E}.....................OK     [max err = 1.613E-14, tol = 1.000E-10]
 checking D*grad{F}.....................OK     [max err = 1.083E-02, tol = 2.000E-02]
 checking dE/dt = 0.....................FAILED [got  2.794E-09 should be   0.000E+00, err = 2.794E-09, tol = 2.500E-09]
FAILED [got  2.794E-09 should be   0.000E+00, err = 2.794E-09, tol = 2.500E-09]
 checking xi(t_10)......................OK     [max err = 4.801E-05, tol = 3.500E-04]
 checking xi(t_20)......................OK     [max err = 2.338E-04, tol = 3.500E-04]
 checking xi(t_30)......................OK     [max err = 2.298E-04, tol = 3.500E-04]
 checking xi(t_40)......................OK     [max err = 2.315E-04, tol = 3.500E-04]
 checking xi(t_50)......................OK     [max err = 2.049E-04, tol = 3.500E-04]
 checking xi(t_60)......................OK     [max err = 2.715E-04, tol = 3.500E-04]
 checking xi(t_70)......................OK     [max err = 2.176E-04, tol = 3.500E-04]
 checking xi(t_80)......................OK     [max err = 8.126E-05, tol = 3.500E-04]
 checking xi(t_90)......................OK     [max err = 1.468E-04, tol = 3.500E-04]
 checking xi(t_**)......................OK     [max err = 1.912E-04, tol = 3.500E-04]
 checking xi(t_**)......................OK     [max err = 1.500E-04, tol = 3.500E-04]
 checking xi(t_**)......................OK     [max err = 1.067E-04, tol = 3.500E-04]

<-- RADIATION TEST COMPLETE
SUMMARY OF ALL TESTS:
PASSED: 156 of 157  99.4%
FAILED:   1 of 157   0.6%

 _____ _    ___ _     
|  ___/ \  |_ _| |    
| |_ / _ \  | || |    
|  _/ ___ \ | || |___ 
|_|/_/   \_\___|_____|

TEST SUITE FAILED

@bjnorfolk
Copy link

Can you pull this @danieljprice ? I can't convert anything to hdf5 currently.

@danieljprice
Copy link
Owner

I think this might just need to be updated from master for the checks to pass. All checks are currently passing on the master branch.

if (fulldump) then
allocate(pressure(npart),beta_pr(npart),dtin(npart))
allocate(pressure(npart),beta_pr(npart),dtin(npart),temperature(npart))
Copy link
Owner

Choose a reason for hiding this comment

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

memory should not be allocated here for either pressure or temperature

call equationofstate(ieos,ponrhoi,spsoundi,rhoi,xyzh(1,i),xyzh(2,i),xyzh(3,i))
endif
pressure(i) = ponrhoi*rhoi
pressure(i) = eos_vars(igasP,i)
Copy link
Owner

Choose a reason for hiding this comment

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

Eos_vars should be written directly to disk, there is no need for intermediate storage

This also adds dim and part as dependencies to utils_dumpfile_hdf5. This
makes sense, as they are low level dependencies relating to particle
arrays.
@dmentipl
Copy link
Contributor

dmentipl commented Sep 4, 2020

@danieljprice: I've implemented the requested changes on my fork (https://github.com/dmentipl/phantom).

I'm not sure if I can add my commits to this PR. I can create a new pull request, if that's OK @dliptai? Or else, if you make me a contributor to your fork, I can push the changes that way.

These files do not require preprocessing so they should have a f90 file
extension not F90.
@dmentipl
Copy link
Contributor

dmentipl commented Sep 4, 2020

All the tests are passing on my fork:

https://github.com/dmentipl/phantom/actions

@danieljprice
Copy link
Owner

@dmentipl can we switch the pull request to your fork? I assume this now works because you merged master into it.

I think easiest way is just to close this one and open a new one from @dmentipl's fork.

@danieljprice
Copy link
Owner

also, just for reference, failures here reflect ongoing problems raised in #46

@dmentipl
Copy link
Contributor

dmentipl commented Sep 4, 2020

@dmentipl can we switch the pull request to your fork? I assume this now works because you merged master into it.

I think easiest way is just to close this one and open a new one from @dmentipl's fork.

Sorry, I just saw this...

I've merged my changes into @dliptai's fork which has added to this pull request. The tests are running now.

@danieljprice danieljprice merged commit 9833784 into danieljprice:master Sep 4, 2020
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.

None yet

4 participants