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

1D GRPIC #59

Open
wants to merge 53 commits into
base: 1.2.0rc
Choose a base branch
from
Open

Conversation

StaticObserver
Copy link
Contributor

I finished tests of new metric, they all passed

@haykh haykh changed the base branch from master to 1.1.0rc July 2, 2024 14:41
@haykh haykh added the enhancement New feature or request label Jul 2, 2024
@haykh
Copy link
Collaborator

haykh commented Jul 2, 2024

@StaticObserver nice! i changed the merge branch to 1.1.0rc. we try to never directly merge to master.

@haykh
Copy link
Collaborator

haykh commented Jul 2, 2024

@StaticObserver i rebased the branch so it's now up-to-date with 1.1.0rc. Only changes relevant to the new metric are visible now.

@haykh haykh self-requested a review July 2, 2024 14:55
@haykh
Copy link
Collaborator

haykh commented Jul 3, 2024

METRICS::fs test currently fails due to likely accuracy issues. numbers are close, but not close enough for the cmp::AlmostEqual. @StaticObserver could you fix and push?

@StaticObserver
Copy link
Contributor Author

The tests all passed on HIP device and failed on Cuda device. I carefully checked the expressions of f0 and find no errors, I simply changed the accuracy requirement from 10 to 30, and they all should pass now.

@StaticObserver
Copy link
Contributor Author

@StaticObserver nice! i changed the merge branch to 1.1.0rc. we try to never directly merge to master.

Sorry, I just found where to change the branch to merge

@haykh
Copy link
Collaborator

haykh commented Jul 3, 2024

The tests all passed on HIP device and failed on Cuda device. I carefully checked the expressions of f0 and find no errors, I simply changed the accuracy requirement from 10 to 30, and they all should pass now.

you mean in GH actions? it actually doesn't run the AMD, which is why the circle is orange. the nvidia ones are ran automatically on my computer whenever i log in (github doesn't provide GPUs for free). i haven't set up the HIP container for gh actions yet, that one will have to run on my laptop.

if you click details on the failed test and scroll down, you should see where it fails. in this case, for instance, it fails at test 11 (double precision):

test 11
      Start 11: METRICS::coord_trans

11: Test command: /home/runner/actions-runner/_work/entity/entity/build/metrics/tests/test-metrics-coord_trans.xc
11: Working Directory: /home/runner/actions-runner/_work/entity/entity/build/metrics/tests
11: Test timeout computed to be: 10000000
11: 0 : 5.000000000000e-01 != 5.000000000000e-01 code->phys not invertible
11: 0 : 1.500000000000e+00 != 1.500000000000e+00 code->phys not invertible
11: 0 : 2.500000000000e+00 != 2.500000000000e+00 code->phys not invertible
11: 0 : 4.500000000000e+00 != 4.500000000000e+00 code->phys not invertible
11: 0 : 5.500000000000e+00 != 5.500000000000e+00 code->phys not invertible
11: 0 : 6.500000000000e+00 != 6.500000000000e+00 code->phys not invertible
11: 0 : 7.500000000000e+00 != 7.500000000000e+00 code->phys not invertible
11: 0 : 9.500000000000e+00 != 9.500000000000e+00 code->phys not invertible
11: 0 : 1.950000000000e+01 != 1.950000000000e+01 code->phys not invertible
11: 0 : 2.050000000000e+01 != 2.050000000000e+01 code->phys not invertible
11: code-sph-phys for 1D flux_surface failed with 10 errors
11/34 Test #11: METRICS::coord_trans .............***Failed    0.27 sec
0 : 5.000000000000e-01 != 5.000000000000e-01 code->phys not invertible
0 : 1.500000000000e+00 != 1.500000000000e+00 code->phys not invertible
0 : 2.500000000000e+00 != 2.500000000000e+00 code->phys not invertible
0 : 4.500000000000e+00 != 4.500000000000e+00 code->phys not invertible
0 : 5.500000000000e+00 != 5.500000000000e+00 code->phys not invertible
0 : 6.500000000000e+00 != 6.500000000000e+00 code->phys not invertible
0 : 7.500000000000e+00 != 7.500000000000e+00 code->phys not invertible
0 : 9.500000000000e+00 != 9.500000000000e+00 code->phys not invertible
0 : 1.950000000000e+01 != 1.950000000000e+01 code->phys not invertible
0 : 2.050000000000e+01 != 2.050000000000e+01 code->phys not invertible
code-sph-phys for 1D flux_surface failed with 10 errors

from the looks of it, it seems the issue is in floating point comparison. unfortunately, this is a very sensitive operation, and often depends on the architecture/compiler etc. so in these tests, i usually try to compare numbers with a large-enough margin of error. i.e., cmp::AlmostEqual(a, b, numeri_limits::epsilon * 1000) or something like that.

@haykh
Copy link
Collaborator

haykh commented Jul 3, 2024

Single precision tests also fail at test 11:

test 11
      Start 11: METRICS::coord_trans

11: Test command: /home/runner/actions-runner/_work/entity/entity/build/metrics/tests/test-metrics-coord_trans.xc
11: Working Directory: /home/runner/actions-runner/_work/entity/entity/build/metrics/tests
11: Test timeout computed to be: 10000000
11: 0 : 5.000000000000e-01 != 4.999967813492e-01 code->phys not invertible
11: 0 : 1.500000000000e+00 != 1.499975204468e+00 code->phys not invertible
11: 0 : 3.500000000000e+00 != 3.499992370605e+00 code->phys not invertible
11: 0 : 4.500000000000e+00 != 4.499970912933e+00 code->phys not invertible
11: 0 : 5.500000000000e+00 != 5.499964237213e+00 code->phys not invertible
11: 0 : 6.500000000000e+00 != 6.500025749207e+00 code->phys not invertible
11: 0 : 8.500000000000e+00 != 8.500012397766e+00 code->phys not invertible
11: 0 : 1.050000000000e+01 != 1.049997711182e+01 code->phys not invertible
11: 0 : 1.150000000000e+01 != 1.150001621246e+01 code->phys not invertible
11: 0 : 1.250000000000e+01 != 1.249997901917e+01 code->phys not invertible
11: code-sph-phys for 1D flux_surface failed with 10 errors
11/34 Test #11: METRICS::coord_trans .............***Failed    0.23 sec
0 : 5.000000000000e-01 != 4.999967813492e-01 code->phys not invertible
0 : 1.500000000000e+00 != 1.499975204468e+00 code->phys not invertible
0 : 3.500000000000e+00 != 3.499992370605e+00 code->phys not invertible
0 : 4.500000000000e+00 != 4.499970912933e+00 code->phys not invertible
0 : 5.500000000000e+00 != 5.499964237213e+00 code->phys not invertible
0 : 6.500000000000e+00 != 6.500025749207e+00 code->phys not invertible
0 : 8.500000000000e+00 != 8.500012397766e+00 code->phys not invertible
0 : 1.050000000000e+01 != 1.049997711182e+01 code->phys not invertible
0 : 1.150000000000e+01 != 1.150001621246e+01 code->phys not invertible
0 : 1.250000000000e+01 != 1.249997901917e+01 code->phys not invertible
code-sph-phys for 1D flux_surface failed with 10 errors

and here the difference is more prominent, but again likely due to truncation error.

@StaticObserver
Copy link
Contributor Author

I tried my best to reduce the truncation error.

@haykh
Copy link
Collaborator

haykh commented Jul 5, 2024

    Inline auto eta2r(const real_t& eta) const -> real_t{
      real_t diff = TWO * math::sqrt(ONE - SQR(a));
      real_t exp_m1 = std::expm1(eta * diff);
      return rh_m - diff / exp_m1;
    }

you cannot use std:: functions, as they don't have CUDA/HIP counterparts. you have to use functions defined by Kokkos, which are architecture-portable: https://kokkos.org/kokkos-core-wiki/API/core/numerics/mathematical-functions.html

so std::expm1 has to be math::expm1 where math:: namespace is an alias of Kokkos::.

@StaticObserver
Copy link
Contributor Author

I see, but they worked well, i guess it was just not efficient?

@haykh haykh added this to the 1D GRPIC milestone Jul 26, 2024
@haykh
Copy link
Collaborator

haykh commented Jul 26, 2024

@StaticObserver all your pushes will be published here automatically. no need for additional PR.

@haykh haykh changed the base branch from 1.1.0rc to 1.2.0rc July 29, 2024 03:35
@StaticObserver
Copy link
Contributor Author

@StaticObserver all your pushes will be published here automatically. no need for additional PR.

I see, thanks for reminding.

@haykh haykh changed the title Adding metric flux_surface and its tests 1D GRPIC Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants