Skip to content

(apr) bug fix#1

Open
chunliangmu wants to merge 2 commits into
becnealon:apr-conservation-bug2from
chunliangmu:apr
Open

(apr) bug fix#1
chunliangmu wants to merge 2 commits into
becnealon:apr-conservation-bug2from
chunliangmu:apr

Conversation

@chunliangmu
Copy link
Copy Markdown

Description:

Fix bug where the enclosed gas mass for sink particles may not be calculated correctly.
This bug is caused by the gas particles' apr mass are mistakenly inferred from sink particles' indexes i instead of gas particles' indexes j.
The bug will affect calculations when using apr and sink particles has heating (e.g. when sink particles has non-zero luminosity).

Components modified:

  • Setup (src/setup)
  • Main code (src/main)
  • Moddump utilities (src/utils/moddump)
  • Analysis utilities (src/utils/analysis)
  • Test suite (src/tests)
  • Documentation (docs/)
  • Build/CI (build/ or github actions)

Type of change:

  • Bug fix
  • Physics improvements
  • Better initial conditions
  • Performance improvements
  • Documentation update
  • Better testing
  • Code cleanup / refactor
  • Other (please describe)

Testing:

Did you run the bots? no

Did you update relevant documentation in the docs directory? no

Did you add comments such that the purpose of the code is understandable? no

Is there a unit test that could be added for this feature/bug? yes

If so, please describe what a unit test might check:

Check that running phantom on APR=yes with a sink particle with large radius and non zero luminosity that the enclosed mass is correct

Related issues: #

@becnealon
Copy link
Copy Markdown
Owner

Thanks @chunliangmu , I think you are correct that this is a bug. Did you run the test suite or check that it compiles with DEBUG=yes?

@chunliangmu
Copy link
Copy Markdown
Author

Hi @becnealon , I checked that it compiles with DEBUG=yes.

On the other hand, when I run the test suite, it fails as it has a larger error on kinetic energy conservation when merging; but that is the same for the magic merging method before my change

--> TESTING APR MODULE

 ---------------- particles set on  20 x  20 x  20 uniform cubic lattice --------------
  x:  -0.500    ->  0.500       y:  -0.500    ->  0.500       z:  -0.500    ->  0.500
 dx:   5.000E-02               dy:   5.000E-02               dz:   5.000E-02
 -----------------------------------------------------------------------------------------


--> adding an apr region

 WARNING! init_apr: resetting split_dir=3 because using multiple regions


--> conducting a split
 checking angular momentum..............OK     [max err = 1.809E-15, tol = 2.000E-14]
 checking linear momentum...............OK     [max err = 0.000E+00, tol = 2.000E-15]
 checking total energy..................OK     [max err = 1.327E-16, tol = 2.000E-15]
 checking kinetic energy................OK     [max err = 2.211E-16, tol = 2.000E-15]
 checking thermal energy................OK     [max err = 1.659E-16, tol = 2.000E-15]

--> conducting a merge
 checking angular momentum..............OK     [max err = 3.818E-15, tol = 2.000E-14]
 checking linear momentum...............OK     [max err = 1.279E-16, tol = 2.000E-15]
 checking total energy.................. FAILED [got  8.366E-01 should be  8.366E-01 ratio = 1.000E+00 err = 2.250E-05 tol = 2.000E-15]
 checking kinetic energy................ FAILED [got  5.021E-01 should be  5.020E-01 ratio = 1.000E+00 err = 3.750E-05 tol = 2.000E-15]
 checking thermal energy................OK     [max err = 3.319E-16, tol = 2.000E-15]
 checking number of particles conserved...........OK     [got       8004 should be        8000, tol =     6]

<-- APR TEST COMPLETE

<-- testing complete
 total wall time =   2.75 s
 total cpu time  =  12.07 s

SUMMARY OF ALL TESTS:
PASSED:   1 of   2  50.0%
FAILED:   1 of   2  50.0%

(for both before and after the bug fix)

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