PyFstat issueshttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues2020-01-21T21:54:02Zhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/23Zenodo release2020-01-21T21:54:02ZDavid Keiteldavid.keitel@ligo.orgZenodo releaseHi @GregAshton , since Pep is planning to contribute some new patches soon, I think now would be a great time to make a Zenodo release off the current top of master (after including the minimum version requirement from !28). This would a...Hi @GregAshton , since Pep is planning to contribute some new patches soon, I think now would be a great time to make a Zenodo release off the current top of master (after including the minimum version requirement from !28). This would also allow us to
* at the same time, address #18 (include @ReinhardPrix as a full author, I think you need to do this in Zenodo metadata)
* *after* the release, bump the version requirement to 3.6, with Debian Stretch users having a sufficiently recent release to fall back toGregory AshtonGregory Ashtonhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/22teach TransientGridSearch to re-use atoms and Fmn from disk2020-02-05T10:56:57ZDavid Keiteldavid.keitel@ligo.orgteach TransientGridSearch to re-use atoms and Fmn from disk`GridSearch` already has logic to re-use results from disk, if inputs match. `TransientGridSearch` inherits this but isn't able to do the same for the actual transient-relevant outputs, just the standard persistent-2F grid file.
Best wa...`GridSearch` already has logic to re-use results from disk, if inputs match. `TransientGridSearch` inherits this but isn't able to do the same for the actual transient-relevant outputs, just the standard persistent-2F grid file.
Best way to do this would probably be through overriding the `check_old_data_is_okay_to_use()` method.David Keiteldavid.keitel@ligo.orgDavid Keiteldavid.keitel@ligo.orghttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/21add manual ephemerides options to search classes2020-02-05T10:55:54ZDavid Keiteldavid.keitel@ligo.orgadd manual ephemerides options to search classesAlready before !25 the Readme was claiming that one can provide ephemerides manually through a search object's arguments. However this doesn't seem to be true: `BaseSearchClass` has a set_ephemeris_files() method, but it's called without...Already before !25 the Readme was claiming that one can provide ephemerides manually through a search object's arguments. However this doesn't seem to be true: `BaseSearchClass` has a set_ephemeris_files() method, but it's called without arguments in __init__(), which itself doesn't take any ephemerides arguments.David Keiteldavid.keitel@ligo.orgDavid Keiteldavid.keitel@ligo.orghttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/20set minimum python3 requirement2020-02-05T10:54:59ZDavid Keiteldavid.keitel@ligo.orgset minimum python3 requirementAs discussed in #17:
David:
> @GregAshton @ReinhardPrix should we just deprecate python 2 for this package (and set an explicit minimum version in the setup), instead of investing energy into maintaining separate package requirements?
...As discussed in #17:
David:
> @GregAshton @ReinhardPrix should we just deprecate python 2 for this package (and set an explicit minimum version in the setup), instead of investing energy into maintaining separate package requirements?
Reinhard:
> Yes, totally, I'm actually for setting the minimum to 3.7 or even the newly-released 3.8 and never looking back. Any python version can easily be installed either via pyenv or conda or similar ...
David:
> 3\.7 could be ok, while 3.8 is a bit too bleeding edge (e.g. no lalsuite wheels are built for it yet, see https://git.ligo.org/lscsoft/lalsuite/issues/212 ); but for the moment I'd prefer even 3.5 as the minimum (to keep Debian Stretch supported).
David:
> I just found out that the black style checker doesn't support 3.5 anymore, so yeah, I definitely have to admit that version is on its way out. I suggest we do one release after my current set of MRs with 3.5 as the minimum requirement, then immediately after bump to 3.6 for any further development work.David Keiteldavid.keitel@ligo.orgDavid Keiteldavid.keitel@ligo.orghttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/19missing zero-initialisation of FstatResults2020-02-05T10:53:27ZDavid Keiteldavid.keitel@ligo.orgmissing zero-initialisation of FstatResultsIn !26 I noticed that 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 ...In !26 I noticed that 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...https://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/18contributors/authors (promote Reinhard)2020-01-22T08:26:58ZDavid Keiteldavid.keitel@ligo.orgcontributors/authors (promote Reinhard)@GregAshton : Currently the readme lists as contributors:
```
Greg Ashton
David Keitel
Reinhard Prix
Karl Wette
Sylvia Zhu
```
But [on Zenodo](https://zenodo.org/record/1243931) only Greg and myself are listed as main authors, and henc...@GregAshton : Currently the readme lists as contributors:
```
Greg Ashton
David Keitel
Reinhard Prix
Karl Wette
Sylvia Zhu
```
But [on Zenodo](https://zenodo.org/record/1243931) only Greg and myself are listed as main authors, and hence make it into the Bibtex export, with the other three as "Researcher(s)" which I guess means something like "contributors in spirit, but not in commits", and doesn't bring actual credit when citing. IMHO, for the next release, we should upgrade @ReinhardPrix to full contributor/author.
I'm not sure if this part of the Zenodo record is extracted from somewhere in the repository, or if you just had to enter it manually when uploading?Gregory AshtonGregory Ashtonhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/17install issues2020-01-14T10:52:51ZDavid Keiteldavid.keitel@ligo.orginstall issuesI'm unable to install from the current version 6057a8edc5efde55b7cfde774c2905acd249c7ad under either python 2 or 3. I'll make a branch for fixing it under py3 first, then see if py2 compatibility is still possible too.I'm unable to install from the current version 6057a8edc5efde55b7cfde774c2905acd249c7ad under either python 2 or 3. I'll make a branch for fixing it under py3 first, then see if py2 compatibility is still possible too.Gregory AshtonGregory Ashtonhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/16make MRs public?2020-01-14T10:52:36ZDavid Keiteldavid.keitel@ligo.orgmake MRs public?Hi @GregAshton is there a reason issues on this project are public, but MRs are only visible to project members? From the settings it looks like one can only allow both seeing+submitting MRs together as a single switch, but both would se...Hi @GregAshton is there a reason issues on this project are public, but MRs are only visible to project members? From the settings it looks like one can only allow both seeing+submitting MRs together as a single switch, but both would seem good to me.Gregory AshtonGregory Ashtonhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/15enable optional use of resampling algorithm2020-02-05T10:52:17ZDavid Keiteldavid.keitel@ligo.orgenable optional use of resampling algorithmAs reported by Pep Covas, we currently seem to have the demod F-Stat algorithm hardcoded. My first suggestion was to just add a new "method" argument to the "ComputeFstat(BaseSearchClass)" class and change the line
```
FstatOAs.FstatMeth...As reported by Pep Covas, we currently seem to have the demod F-Stat algorithm hardcoded. My first suggestion was to just add a new "method" argument to the "ComputeFstat(BaseSearchClass)" class and change the line
```
FstatOAs.FstatMethod = lalpulsar.FstatOptionalArgsDefaults.FstatMethod
```
in core.py accordingly.
Pep tested something like this and got:
```
XLAL Error - XLALComputeFstatResamp (ComputeFstat_Resamp.c:470): Resampling does not currently support atoms per detector
XLAL Error - XLALComputeFstatResamp (ComputeFstat_Resamp.c:470): Invalid argument
XLAL Error - XLALComputeFstat (ComputeFstat.c:828): Check failed: (input->method_funcs.compute_func) ( *Fstats, common, input->method_data ) == XLAL_SUCCESS
XLAL Error - XLALComputeFstat (ComputeFstat.c:828): Internal function call failed: Invalid argument
```
So the most difficult part would be to look into how the semicoherent search could be adapted to not use the atoms flag.https://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/14Version mismatch between examples2020-02-05T10:50:58ZGregory AshtonVersion mismatch between examplesThe example `semi_coherent_search_using_MCMC.py ` is currently broken with the error message
```
../lib/python2.7/site-packages/pyfstat/mcmc_based_searches.py in run(self, proposal_scale_factor, create_plots, window, **kwargs)
526 ...The example `semi_coherent_search_using_MCMC.py ` is currently broken with the error message
```
../lib/python2.7/site-packages/pyfstat/mcmc_based_searches.py in run(self, proposal_scale_factor, create_plots, window, **kwargs)
526 logl=self.logl, logp=self.logp,
527 logpargs=(self.theta_prior, self.theta_keys, self.search),
--> 528 loglargs=(self.search,), betas=self.betas, a=proposal_scale_factor)
529
530
TypeError: __init__() got an unexpected keyword argument 'a'
```Gregory AshtonGregory Ashtonhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/13Potentially dangerous (on clusters) temp-file naming policy in Writer.predict...2020-02-05T10:48:39ZReinhard PrixPotentially dangerous (on clusters) temp-file naming policy in Writer.predict_Fstat()In debugging some confusing behavior/problems of pyFstat on a cluster, I noticed that the method predict_fstat() of the class Writer, in make_sfts.py:257 uses the 'label' argument to construct the filename for the temporary internal fil...In debugging some confusing behavior/problems of pyFstat on a cluster, I noticed that the method predict_fstat() of the class Writer, in make_sfts.py:257 uses the 'label' argument to construct the filename for the temporary internal file ('temporyFile') in the *user's home-dir* they launched the jobs from.
This is potentially confusing/dangerous, as it's not documented (I believe) and as there's also the argument 'outdir', which is more obvious to be required to be unique on cluster runs.
What can happen here is that someone uses the same label for all jobs of a cluster run, gives every job a unique 'outdir', and still jobs will fall over themselves when all trying to read-read-delete the same temporary file in predict_fstat().
As as fix I would simply suggest to place that temporary file inside the 'outdir' as well, then there should be no problems.Gregory AshtonGregory Ashtonhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/12test failures on CIT2020-01-14T10:52:01ZDavid Keiteldavid.keitel@ligo.orgtest failures on CITI'm trying to get a PyFstat installation running on CIT, and getting test failures. (Though some basic examples to work already.)
1. something related to matplotlib backends again, MR incoming:
`[david.keitel@ldas-pcdev13 PyFstat]$ pyt...I'm trying to get a PyFstat installation running on CIT, and getting test failures. (Though some basic examples to work already.)
1. something related to matplotlib backends again, MR incoming:
`[david.keitel@ldas-pcdev13 PyFstat]$ python tests.py
Traceback (most recent call last):
File "tests.py", line 5, in <module>
import pyfstat
File "/home/david.keitel/git/PyFstat/pyfstat/__init__.py", line 3, in <module>
from .core import BaseSearchClass, ComputeFstat, SemiCoherentSearch, SemiCoherentGlitchSearch
File "/home/david.keitel/git/PyFstat/pyfstat/core.py", line 15, in <module>
import pyfstat.helper_functions as helper_functions
File "/home/david.keitel/git/PyFstat/pyfstat/helper_functions.py", line 20, in <module>
import matplotlib.pyplot as plt
File "/home/david.keitel/.local/lib/python2.7/site-packages/matplotlib/pyplot.py", line 116, in <module>
_backend_mod, new_figure_manager, draw_if_interactive, _show = pylab_setup()
File "/home/david.keitel/.local/lib/python2.7/site-packages/matplotlib/backends/__init__.py", line 60, in pylab_setup
[backend_name], 0)
File "/home/david.keitel/.local/lib/python2.7/site-packages/matplotlib/backends/backend_tkagg.py", line 6, in <module>
from six.moves import tkinter as Tk
File "/home/david.keitel/.local/lib/python2.7/site-packages/six.py", line 203, in load_module
mod = mod._resolve()
File "/home/david.keitel/.local/lib/python2.7/site-packages/six.py", line 115, in _resolve
return _import_module(self.mod)
File "/home/david.keitel/.local/lib/python2.7/site-packages/six.py", line 82, in _import_module
__import__(name)
ImportError: No module named Tkinter
`
2. After fixing that one, some `Directory not empty` or `Device or resource busy` errors in test 12. Full log attached. Might be a shutil version issue?[test_ldas-pcdev13_20180111.log](/uploads/2e28da6129b662f65952dfcbcc60dbed/test_ldas-pcdev13_20180111.log)Gregory AshtonGregory Ashtonhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/11Move transient gridded search to separate class2020-01-14T10:51:35ZGregory AshtonMove transient gridded search to separate classHi @dkeitel ,
Currently, I think you are using `GridSearch` to run your transient grid searches, is that correct? If you continue to develop this, it may make sense to create a specific subclass, say `TransientGridSearch`. Basically, yo...Hi @dkeitel ,
Currently, I think you are using `GridSearch` to run your transient grid searches, is that correct? If you continue to develop this, it may make sense to create a specific subclass, say `TransientGridSearch`. Basically, you do something like
```
class TransientGridSearch(GridSearch)
def __init__ ...
```
This way, `TransientGridSearch` inherits all methods from `GridSearch`, but then overwrite any parts it needs to modify.
The benefit is, any other methods which subclass `GridSearch`, doesn't need to worry about the transient parts (and hence changes to the transient parts won't break anything else).
Is there any reason why this wouldn't work? If not, I'll have a look at creating a patch and seeing what you thinkGregory AshtonGregory Ashtonhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/10reminder: add header to output files2020-02-05T10:47:15ZDavid Keiteldavid.keitel@ligo.orgreminder: add header to output filesLow-priority reminder issue for text file headers, as briefly discussed in #8:
> David: generally add a header with all search parameters (also beneficial for data archiving and external postprocessing
> Greg: Nevertheless, adding a ...Low-priority reminder issue for text file headers, as briefly discussed in #8:
> David: generally add a header with all search parameters (also beneficial for data archiving and external postprocessing
> Greg: Nevertheless, adding a header as is done by CFSv2 is something on my to-do list (also for the MCMC output)David Keiteldavid.keitel@ligo.orgDavid Keiteldavid.keitel@ligo.orghttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/9grid_examples/grid_F0F1F2.py import: projection_matrix ?2020-01-14T10:50:56ZDavid Keiteldavid.keitel@ligo.orggrid_examples/grid_F0F1F2.py import: projection_matrix ?I tried to run another example, grid_examples/grid_F0F1F2.py and (just for plotting at the end) it tries to import "projection_matrix", which I don't have and couldn't find through pip. What's that package? Or is it a local file you forg...I tried to run another example, grid_examples/grid_F0F1F2.py and (just for plotting at the end) it tries to import "projection_matrix", which I don't have and couldn't find through pip. What's that package? Or is it a local file you forgot to commit?Gregory AshtonGregory Ashtonhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/8gridsearch output format and (t0,tau)band use case2020-01-14T10:51:26ZDavid Keiteldavid.keitel@ligo.orggridsearch output format and (t0,tau)band use caseFor the implementation of explicit t0,tau search bands in !7 I'm a bit in conflict with PyFstat's output file format from gridded searches, which so far is a simple headerless file with columns
`startTime endTime F0 F1 F2 Alpha Delta 2F`...For the implementation of explicit t0,tau search bands in !7 I'm a bit in conflict with PyFstat's output file format from gridded searches, which so far is a simple headerless file with columns
`startTime endTime F0 F1 F2 Alpha Delta 2F`.
First a question: the original transient search style you had in mind was probably to pass arrays of minStartTime, maxStartTime and then the search is an (up to) 7D grid of ComputeFStat calls over all those columns? That's how I parse GridSearch.get_input_data_array too.
Whereas the !7 version is closer to CFSv2, max-5D search and transient postprocessing over a t0,tau grid at each point. I can see value in both approaches and we should be able to support both in the same interface and output format.
In the !7 implementation, it now still writes out the same overall minStartTime and maxStartTime into the first 2 columns for every Doppler candidate, and in the last column max(2F) maximised over the t0,tau grid. Problem with that is information loss, as a symptom of which running a new GridSearch with added transientWindowType, t0Band, tauBand arguments in a directory with an existing output file from identical Doppler parameters will not trip check_old_data_is_okay_to_use and hence not actually rerun the search.
Simple solutions:
1. in the (t0Band,tauBand) usecase, replace the first 2 columns by the ML estimators (t0,t0+tau at which F=maxF)
1. in the (t0Band,tauBand) usecase, add extra columns
1. generally add a header with all search parameters (also beneficial for data archiving and external postprocessing)
More complicated solution:
1. wrap lalpulsar file-writing functions
What do you think @GregAshton ?David Keiteldavid.keitel@ligo.orgDavid Keiteldavid.keitel@ligo.orghttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/7add switches to override check_old_data_is_okay_to_use?2020-01-14T10:50:41ZDavid Keiteldavid.keitel@ligo.orgadd switches to override check_old_data_is_okay_to_use?Both the grid and MCMC search classes have check_old_data_is_okay_to_use methods, which are a good idea in principle, but might need to get quite a bit more involved to check all possible differences between search calls in all use cases...Both the grid and MCMC search classes have check_old_data_is_okay_to_use methods, which are a good idea in principle, but might need to get quite a bit more involved to check all possible differences between search calls in all use cases. (E.g. the simple grid output format currently cannot be used to distinguish between a search with and without t0,tau bands, which I'll open another issue about.)
So a nice convenience feature, at least for use during development, would be a 'reusedata' argument, default True, but when set to False just skips that check. If you like it, I can make a patch.Gregory AshtonGregory Ashtonhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/6example failures2020-01-14T10:50:29ZDavid Keiteldavid.keitel@ligo.orgexample failuresHi @GregAshton, two issues I found with running examples on current master, the first in transient_search_using_MCMC.py and apparently related to the ptemcee move and hence I'm not sure how to debug it:
`12:12 INFO : Running final bu...Hi @GregAshton, two issues I found with running examples on current master, the first in transient_search_using_MCMC.py and apparently related to the ptemcee move and hence I'm not sure how to debug it:
`12:12 INFO : Running final burn and prod with 200 steps
0%| | 0/200 [00:00<?, ?it/s]Traceback (most recent call last):
File "examples/other_examples/transient_search_using_MCMC.py", line 47, in <module>
mcmc.run()
File "/home/dkeitel/git/PyFstat/pyfstat/mcmc_based_searches.py", line 522, in run
sampler = self._run_sampler(sampler, p0, nburn=nburn, nprod=nprod)
File "/home/dkeitel/git/PyFstat/pyfstat/mcmc_based_searches.py", line 388, in _run_sampler
total=nburn+nprod):
File "/home/dkeitel/.local/lib/python2.7/site-packages/tqdm/_tqdm.py", line 862, in __iter__
for obj in iterable:
File "/home/dkeitel/.local/lib/python2.7/site-packages/ptemcee/sampler.py", line 355, in sample
raise ValueError('Attempting to start with samples outside posterior support.')
ValueError: Attempting to start with samples outside posterior support.
`
Probably more trivial to fix is fully_coherent_search_using_MCMC.py which runs mostly through but gives at the end:
`Traceback (most recent call last):
File "fully_coherent_search_using_MCMC.py", line 60, in <module>
mcmc.run(subtractions=[F0, F1])
File "/home/dkeitel/git/PyFstat/pyfstat/mcmc_based_searches.py", line 524, in run
fig, axes = self._plot_walkers(sampler, nprod=nprod, **kwargs)
TypeError: _plot_walkers() got an unexpected keyword argument 'subtractions'`
Is it safe to just remove that argument, or is there a replacement?Gregory AshtonGregory Ashtonhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/5git clone URL in readme: https instead of ssh?2020-01-14T10:50:14ZDavid Keiteldavid.keitel@ligo.orggit clone URL in readme: https instead of ssh?Hi Greg, Simone asked me for help in cloning the repo today. The readme recommends
`$ git clone git@gitlab.aei.uni-hannover.de:GregAshton/PyFstat.git`
which requires an account and public key, whereas
`git clone https://gitlab.aei.uni-ha...Hi Greg, Simone asked me for help in cloning the repo today. The readme recommends
`$ git clone git@gitlab.aei.uni-hannover.de:GregAshton/PyFstat.git`
which requires an account and public key, whereas
`git clone https://gitlab.aei.uni-hannover.de/GregAshton/PyFstat.git`
should work anonymously (and did for him).
Sounds good?Gregory AshtonGregory Ashtonhttps://gitlab.aei.uni-hannover.de/GregAshton/PyFstat/-/issues/4Confusion between F and 2F2020-01-14T10:50:23ZGregory AshtonConfusion between F and 2FAs pointed out by David:
> Meanwhile, I noticed something in the core that made me pause: run_computefstatistic_single_point() returns 2F.
> MCMCTransientSearch.logl() calls this "FS" and does "return FS + self.likelihoodcoef" where fr...As pointed out by David:
> Meanwhile, I noticed something in the core that made me pause: run_computefstatistic_single_point() returns 2F.
> MCMCTransientSearch.logl() calls this "FS" and does "return FS + self.likelihoodcoef" where from L~exp(F) I think it should >be F not 2F...?
>(Same for the base MCMC class, it just seems to have another wrapper function inbetween.)
>I guess the sampler only ever needs relative likelihoods, anyway, so it doesn't make a difference...?
This needs to be cleaned upGregory AshtonGregory Ashton