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

fix thinko in option parsing for defaults #4615

Merged
merged 2 commits into from
Feb 2, 2024
Merged

Conversation

tdent
Copy link
Contributor

@tdent tdent commented Feb 1, 2024

When an option is not given at all getattr on the args object gives None, but we don't want to translate that into "--option-name None" on the subprocess command line.

When an option is not given at all getattr on the args object gives None, but we don't want to translate that into "--option-name None" on the command line.
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.

Looks good to me. Thanks for fixing it.

@titodalcanton
Copy link
Contributor

Note that the same code will also break if we add an option that takes a list of values (as in nargs='+'). Perhaps worth adding a FIXME or WARNING comment?

obviously we needed to define 'key_name' first ..
@tdent
Copy link
Contributor Author

tdent commented Feb 1, 2024

This is fairly clunky customized code to deal with the specific way the optimizer args are set up .. as long as it stays in the optimizer.py module I think it's OK to fix the multiple args case as and when it ever occurs (it may never do)

@titodalcanton titodalcanton merged commit 7774496 into master Feb 2, 2024
64 checks passed
@titodalcanton titodalcanton deleted the tdent-option-none branch February 2, 2024 09:50
titodalcanton pushed a commit to titodalcanton/pycbc that referenced this pull request Feb 2, 2024
* fix thinko in option parsing for defaults

When an option is not given at all getattr on the args object gives None, but we don't want to translate that into "--option-name None" on the command line.

* bugfix 

obviously we needed to define 'key_name' first ..
titodalcanton added a commit that referenced this pull request Feb 6, 2024
* Remove reference to relative_example in docs

* Use RTLD_GLOBAL for libgomp (#4353)

* Use RTLD_GLOBAL for libgomp

In conda-forge/pycbc-feedstock#74 it was suggested to use RTLD_GLOBAL for libgomp. Let's see if this works fine with the test suite (which should answer @josh-willis 's concerns).

* Move import of ctypes/gomp into __enter__

* Try this

* Revert "Revert "Allow SNR optimizer to use candidate point in initial array (#4393)""

This reverts commit 7be12f1.

We are now catching up with master, where the bug originally introduced
by #4393 is fixed properly, so here I am undoing the temporary fix.

* SNR optimisation options for pycbc_live (#4432)

* Moving the live optimizer option changes to my own branch

* Completing the snr optimization argument group

* updating pycbc_live

* re-adding bug fix

* removing TODO message

* Bug with d_e options

* Adding optimizer-seed

* fixing the d_e optimizer

* replacing run.sh code

* resolve merge conflict

* fixing run.sh

* cleaning up args_to_string func

* changing comment

* codeclimate fixes

* module docstring

* Update module docstring copyright

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>

* Add gareth

* removing argv

* argument changing

* removing duplicated arguments

* minor CC points

* remove bug introduced when making CC happier

---------

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>
Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* Improvements to single-detector trigger fitting code for PyCBC Live (#4486)

* Cleanup

* Cleanup

* Refactor duration bin parsing code and add support for reading from bank

* Minor fix/cleanup to logging

* Update CLI checks for duration bins

* Cleanup

* Ignore inconsistent config when combining

* Fix bug

* Fix typo

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>

* Comment from Gareth

---------

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>

* [pycbc live] Don't add snr options to command if they don't exist (#4518)

* Don't run snr optimizer setup if not optimizing snr

* moving the check to a more appropraite place

* setting snr_opt_options to None if not optimizing

* [pycbc live] Allowing the use of psd variation in the ranking statistic for pycbc live (#4533)

* Modifying files to include psd variation in single detector statistic calculation

* ending variation.py with a blank line

* Changing to an increment agnostic solution

* removing change already fixed

* Updating function names and docstrings

* removing ToDos and adding more helpful comments

* Removing unused import

* Codeclimate fixes

* Removing excess logging and whitespace mistakes

* Removing unused objects + codeclimate fixes

* Updating comments and docstrings, removing matchedfilter changes

* Revert "Updating comments and docstrings, removing matchedfilter changes"

This reverts commit 0e6473a.

* Removing matchedfilter changes, updating comments and docstrings

* Move --verbose to the end of the commands

* more comment updates

* Repositioning filter recreation

* Changes to comments and removing whitespace

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* removing refchecks

* Adding option veification for psd variation

* Apply suggestions from code review

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* fixing EOL error

* Refactoring the filter creation function

* codeclimate fixes

* undo

* full_filt func

* removing indentation

* code climate

* code climate

* try to quiet codeclimate

* codeclimate doesn't know PEP8

* brackets obviate line continuation

---------

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* added scaling of initial pop in snr_optimizer (#4561)

* added scaling of initial pop

* init popn in optimize_di & pso func

* added changes in optimize_pso

* usig logging.debug for snr

* Do not set matplotlib's backend in internal modules (#4592)

* Set version to 2.1.4

* Remove reference to single_template_examples in docs

* Remove reference to hierarchical_model in docs

* Live: produce empty trigger fit plot for detectors with no triggers (#4600)

* Live: produce empty trigger fit plot for detectors with no triggers

* allow for below-threshold triggers

* fix thinko in option parsing for defaults (#4615)

* fix thinko in option parsing for defaults

When an option is not given at all getattr on the args object gives None, but we don't want to translate that into "--option-name None" on the command line.

* bugfix 

obviously we needed to define 'key_name' first ..

* Improvements to single fit plots (#4509)

* Improvements to single fit plots

* Apply suggestions from Gareth

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>

---------

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>

---------

Co-authored-by: Ian Harry <ian.harry@ligo.org>
Co-authored-by: Arthur Tolley <32394213+ArthurTolley@users.noreply.github.com>
Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>
Co-authored-by: Thomas Dent <thomas.dent@usc.es>
Co-authored-by: Praveen Kumar <86048588+PRAVEEN-mnl@users.noreply.github.com>
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Mar 4, 2024
* fix thinko in option parsing for defaults

When an option is not given at all getattr on the args object gives None, but we don't want to translate that into "--option-name None" on the command line.

* bugfix 

obviously we needed to define 'key_name' first ..
lpathak97 pushed a commit to lpathak97/pycbc that referenced this pull request Mar 13, 2024
* fix thinko in option parsing for defaults

When an option is not given at all getattr on the args object gives None, but we don't want to translate that into "--option-name None" on the command line.

* bugfix 

obviously we needed to define 'key_name' first ..
acorreia61201 pushed a commit to acorreia61201/pycbc that referenced this pull request Apr 4, 2024
* fix thinko in option parsing for defaults

When an option is not given at all getattr on the args object gives None, but we don't want to translate that into "--option-name None" on the command line.

* bugfix 

obviously we needed to define 'key_name' first ..
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.

3 participants