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

non-critical: missing zero-initialisation of FstatResults #11

Closed
dbkeitel opened this issue Feb 5, 2020 · 4 comments
Closed

non-critical: missing zero-initialisation of FstatResults #11

dbkeitel opened this issue Feb 5, 2020 · 4 comments
Assignees
Labels
bug Something isn't working core issues on the library core features

Comments

@dbkeitel
Copy link
Collaborator

dbkeitel commented Feb 5, 2020

from https://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/issues/19 (20191105):

if self.whatToCompute = lalpulsar.FSTATQ_2F isn't set, F-statistic output will be nasty uninitialised random numbers. It seems we're not initialising the FstatResults struct properly. Not sure what the proper way to do so through SWIG is...

@dbkeitel dbkeitel added bug Something isn't working core issues on the library core features labels Feb 5, 2020
@dbkeitel dbkeitel added this to the review start milestone Feb 5, 2020
@dbkeitel dbkeitel self-assigned this Feb 5, 2020
@dbkeitel
Copy link
Collaborator Author

dbkeitel commented Mar 4, 2020

Simple hack-ish steps to reproduce:

  1. In core.py, method init_computefstatistic_single_point() of the ComputeFstat class, change
-        self.whatToCompute = lalpulsar.FSTATQ_2F
+        #self.whatToCompute = lalpulsar.FSTATQ_2F
+        self.whatToCompute += lalpulsar.FSTATQ_ATOMS_PER_DET
  1. Do
from tests import ComputeFstat
test=ComputeFstat()
test.setUpClass()
test.test_run_computefstatistic_single_point_injectSqrtSX()

==>

13:49 INFO    : Initialising FstatResults
-6.4088294e+34
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/dkeitel/git/sources/PyFstat/tests.py", line 201, in test_run_computefstatistic_single_point_injectSqrtSX
    self.assertTrue(FS > 0.0)
  File "/usr/lib/python3.7/unittest/case.py", line 692, in assertTrue
    raise self.failureException(msg)
AssertionError: False is not true

@dbkeitel
Copy link
Collaborator Author

dbkeitel commented Mar 4, 2020

This looks like an upstream problem with the SWIG bindings not vetoing access to "illegal" fields of the results structure. I've contacted Karl for it.

In the meantime, this poses no practical problems for PyFstat at all unless one deliberately hacks out the FSTATQ_2F bit from the base ComputeFstat class as done above.

I only noticed this because of an earlier bug where the atoms bit for the transient case was overwriting this bit instead of being added to it, which I've long since fixed.

So this is really just a paranoia-level investigation.

@dbkeitel dbkeitel changed the title missing zero-initialisation of FstatResults non-critical: missing zero-initialisation of FstatResults Mar 4, 2020
@dbkeitel dbkeitel removed this from the review start milestone Mar 4, 2020
@dbkeitel
Copy link
Collaborator Author

Will be solved upstream in lalsuite, thanks to quick work from Karl: https://git.ligo.org/lscsoft/lalsuite/-/merge_requests/1258

@dbkeitel
Copy link
Collaborator Author

This has been solved by @kwwette in the LALSuite SWIG modules, thanks!

https://git.ligo.org/lscsoft/lalsuite/-/commit/c11cd5a0c64de48bc3aced217aa8508a4785eb46

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core issues on the library core features
Projects
None yet
Development

No branches or pull requests

1 participant