basnijholt / pfapack

Efficient numerical computation of the Pfaffian for dense and banded skew-symmetric matrices
https://arxiv.org/abs/1102.3440
Other
14 stars 6 forks source link

fixed bugs integer values matrices #4

Closed gdelvecchiodelvecchio closed 3 years ago

gdelvecchiodelvecchio commented 3 years ago

There were problems with integer valued matrices due to type casting mainly. All the problems fixed now.

basnijholt commented 3 years ago

Thanks a lot for these fixes!

Would you mind deleting/untracking those .idea files?

gdelvecchiodelvecchio commented 3 years ago

You are welcome! Done!

basnijholt commented 3 years ago

@gdelvecchiodelvecchio, I rewrote the tests you made to use the loop variable: https://github.com/basnijholt/pfapack/blob/master/tests/test_int_mats.py#L44

It seems like the epsilon becomes larger. Do you understand why?

basnijholt commented 3 years ago

Here you can see the failing tests: https://github.com/basnijholt/pfapack/runs/3460624581

  =================================== FAILURES ===================================
  ____________________________ test_integer_matrices _____________________________

      def test_integer_matrices():
          for i in range(2, 12, 2):
              A = int_rand_mat(i)
              H = pf.pfaffian(A, method="H")
              P = pf.pfaffian(A, method="P")
              result_H = np.abs(H ** 2 - np.linalg.det(A))
              result_P = np.abs(P ** 2 - np.linalg.det(A))
              eps = 10 ** (-i)
  >           assert result_H < eps
  E           assert 7.450580596923828e-08 < 1e-10

  tests/test_int_mats.py:33: AssertionError
  ______________________________ test_real_matrices ______________________________

      def test_real_matrices():
          for i in range(2, 12, 2):
              A = float_rand_mat(i)
              H = pf.pfaffian(A, method="H")
              P = pf.pfaffian(A, method="P")
              result_H = np.abs(H ** 2 - np.linalg.det(A))
              result_P = np.abs(P ** 2 - np.linalg.det(A))
              eps = 10 ** (-i)
  >           assert result_H < eps
  E           assert 1.4156103134155273e-07 < 1e-10

  tests/test_int_mats.py:45: AssertionError
  ____________________________ test_complex_matrices _____________________________

      def test_complex_matrices():
          for i in range(2, 12, 2):
              A = complex_rand_mat(i)
              H = pf.pfaffian(A, method="H")
              P = pf.pfaffian(A, method="P")
              result_H = np.abs(H ** 2 - np.linalg.det(A))
              result_P = np.abs(P ** 2 - np.linalg.det(A))
              eps = 10 ** (-i)
  >           assert result_H < eps
  E           assert 4.938539383071509e-07 < 1e-08

  tests/test_int_mats.py:57: AssertionError
gdelvecchiodelvecchio commented 3 years ago

I did the following: clone the forked repo from my page locally and run my tests and they work. I am running on windows. Your tests only seem to test ubuntu and macos and this might be the issue. Also, @basnijholt added minor changes to the test file that are a bit different from mines I thought they had been run before being modified. Let me know! Suggest to do the same to @mishmash

basnijholt commented 3 years ago

Thanks for your reply!

In your tests you didn't use the loop variable, so you were only testing for small sizes. Was it not supposed to be that way?

Otherwise I should not have changed anything, except for adding an assert.

Your tests only seem to test ubuntu and macos and this might be the issue

I am >99% sure that this is not the case.

gdelvecchiodelvecchio commented 3 years ago

The problem is related to the fact in the tests I generate random matrices whose determinant is (exponentially) unbounded as a function of their size. Measuring the difference of the logs reveals that all works fine for all sizes! I have pushed the mods in my repo shall I open a pull request?

basnijholt commented 3 years ago

Thanks @gdelvecchiodelvecchio, I copied your changes and simplified the tests.