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

SNR optimizer option types #4650

Merged
merged 2 commits into from
Feb 23, 2024
Merged

SNR optimizer option types #4650

merged 2 commits into from
Feb 23, 2024

Conversation

tdent
Copy link
Contributor

@tdent tdent commented Feb 23, 2024

Fix issue in non-default settings for some options that Max complained about

Fix issue in non-default settings for some options that Max complained about
Copy link
Contributor

@ArthurTolley ArthurTolley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested this change and it fixes the previous TypeError: can't multiply sequence by non-int of type 'float' error. Happy to approve

@tdent
Copy link
Contributor Author

tdent commented Feb 23, 2024

Was that for the time window option or f-lower ?

@ArthurTolley
Copy link
Contributor

@tdent I can re-run the test again to find the exact error but I simply gave --snr-opt-extra-opts \ "--chirp-time-f-lower 20.0 --chirp-time-window 2.0 " \ and the error was there before the fix and not there after the fix :), like I said, I will re-run this in a minute.

@ArthurTolley
Copy link
Contributor

This is the full error:

Traceback (most recent call last):
  File "/home/arthur.tolley/.conda/envs/live_rank_stat/bin/pycbc_optimize_snr", line 141, in <module>
    tau0 = cv.tau0_from_mass1_mass2(fp['mass1'][()], fp['mass2'][()], tau0flow)
  File "/home/arthur.tolley/.conda/envs/live_rank_stat/lib/python3.9/site-packages/pycbc/conversions.py", line 364, in tau0_from_mass1_mass2
    return tau0_from_mtotal_eta(mtotal, eta, f_lower)
  File "/home/arthur.tolley/.conda/envs/live_rank_stat/lib/python3.9/site-packages/pycbc/conversions.py", line 337, in tau0_from_mtotal_eta
    return _a0(f_lower) / (mtotal**(5./3.) * eta)
  File "/home/arthur.tolley/.conda/envs/live_rank_stat/lib/python3.9/site-packages/pycbc/conversions.py", line 322, in _a0
    return 5. / (256. * (numpy.pi * f_lower)**(8./3.))
TypeError: can't multiply sequence by non-int of type 'float'

So f_lower.

@tdent
Copy link
Contributor Author

tdent commented Feb 23, 2024

Yup, f_lower gets multiplied whereas the window gets subtracted or added ;)
Now I just don't know what is causing the 'run small workflow/search' things to fail ...

@ArthurTolley
Copy link
Contributor

For the CI, maybe related to the pegasus upgrade?

Just for completeness, when applying the fix to f_lower and not chirp_time_window we get another error which is also fixed by this explicit type :)

Traceback (most recent call last):
  File "/home/arthur.tolley/.conda/envs/live_rank_stat/bin/pycbc_optimize_snr", line 143, in <module>
    mintau0 = max(tau0 - args.chirp_time_window,
numpy.core._exceptions._UFuncNoLoopError: ufunc 'subtract' did not contain a loop with signature matching types (dtype('float64'), dtype('<U3')) -> None

Copy link
Contributor

@maxtrevor maxtrevor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are improving the command line options, can we please change the verbosity option to use https://github.com/gwastro/pycbc/blob/master/pycbc/__init__.py#L69 ?

@tdent
Copy link
Contributor Author

tdent commented Feb 23, 2024

Max if you make a code suggestion (via the comment interface) we can accept it, although the verbose option is not related to this specific fix

@maxtrevor
Copy link
Contributor

made the requested change myself, and somehow it seems to have gone through without needing to be accepted?

@tdent
Copy link
Contributor Author

tdent commented Feb 23, 2024

Oh, yeah, github does let you edit someone else's PR just like that ..

@tdent tdent enabled auto-merge (squash) February 23, 2024 17:58
@tdent tdent merged commit 6b5fe83 into master Feb 23, 2024
59 of 64 checks passed
@tdent tdent deleted the tdent-opt-float branch February 23, 2024 18:16
@titodalcanton
Copy link
Contributor

Remember to apply the right label to the PR, otherwise this risks not being deployed in the 2.1 release line.

bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Mar 4, 2024
* SNR optimizer option types

Fix issue in non-default settings for some options that Max complained about

* Use standard verbosity argument

---------

Co-authored-by: maxtrevor <65971534+maxtrevor@users.noreply.github.com>
lpathak97 pushed a commit to lpathak97/pycbc that referenced this pull request Mar 13, 2024
* SNR optimizer option types

Fix issue in non-default settings for some options that Max complained about

* Use standard verbosity argument

---------

Co-authored-by: maxtrevor <65971534+maxtrevor@users.noreply.github.com>
maxtrevor added a commit to maxtrevor/pycbc that referenced this pull request Mar 26, 2024
* SNR optimizer option types

Fix issue in non-default settings for some options that Max complained about

* Use standard verbosity argument

---------

Co-authored-by: maxtrevor <65971534+maxtrevor@users.noreply.github.com>
acorreia61201 pushed a commit to acorreia61201/pycbc that referenced this pull request Apr 4, 2024
* SNR optimizer option types

Fix issue in non-default settings for some options that Max complained about

* Use standard verbosity argument

---------

Co-authored-by: maxtrevor <65971534+maxtrevor@users.noreply.github.com>
GarethCabournDavies added a commit that referenced this pull request Apr 19, 2024
* SNR optimizer option types (#4650)

* SNR optimizer option types

Fix issue in non-default settings for some options that Max complained about

* Use standard verbosity argument

---------

Co-authored-by: maxtrevor <65971534+maxtrevor@users.noreply.github.com>

* Need to use atol for isclose here (#4655)

* Update how live fitting deals with invalid values (#4653)

* Update how live fitting deals with invalid values

* TDC comments

* Tito's fixes

* Add check for nan trigger fit values in the singles IFAR calculation

* Properly fix ASD plot minimum value (#4671)

* Properly fix ASD plot minimum value

Rather than a hard coded minimum use the minimum value we are going to plot.  Also put the calculation of the asd we are plotting on a single line rather than splitting over 2

* Fix bug and grid

* ignore samples above minimum freq rather than above 0

* curr_psd does exist !

* specify an older branch of BBHx in tox.ini (#4672) (#4674)

* specify an older branch of BBHx in tox.ini

* Update tox.ini

Co-authored-by: Shichao Wu <wushichao@mail.bnu.edu.cn>

---------

Co-authored-by: Shichao Wu <wushichao@mail.bnu.edu.cn>

* Modify logging in pycbc_optimize_snr to avoid dependencies

* Add live singles significance fitting supervision script to github repo (#4687)

* tidy up code before adding to github repo

* log starting fitting script

* Fix IndexError in PyCBC Live's followup code (#4676)

* Draft fix for followup indexing error

* Use a nonzero min required lookback, better variable names

* Alex's comment on td_samples vs delta_f

* Add docstring for `followup_event_significance`

* Docstring fixes

* Work on comments

* Try to fix Sphinx error

* Simplify definition of strain buffer length

* Explicit kwarg name

Co-authored-by: maxtrevor <65971534+maxtrevor@users.noreply.github.com>

* Simplify `from_cli()` args

* Fix error

* Update example

---------

Co-authored-by: maxtrevor <65971534+maxtrevor@users.noreply.github.com>

* Supervision script typo (#4696)

* increment version

* Fix import broken by change in lscsoft-glue (#4662)

* Drop lscsoft-glue dependency

* Glue still needed, do not drop it!

* Fix bug from scipy version changing (#4683)

---------

Co-authored-by: Thomas Dent <thomas.dent@usc.es>
Co-authored-by: Ian Harry <ian.harry@ligo.org>
Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>
Co-authored-by: Shichao Wu <wushichao@mail.bnu.edu.cn>
Co-authored-by: Tito Dal Canton <tito.dalcanton@ijclab.in2p3.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants