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

Remove pycbc_coinc_time #4822

Merged
merged 2 commits into from
Jul 27, 2024
Merged

Conversation

duncanmmacleod
Copy link
Member

Standard information about the request

This is a: other - backwards-incompatible script removal

This change affects: nothing?

This change changes: unknown

This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines

This change will: break current functionality, require a new release

Motivation

The pycbc_coinc_time script is the only usage of the dqsegdb library in this project. The dqsegdb library is dying and doesn't support Python 3.12.

Quoting private communication from @spxiwh:

That script is abandonware, it almost certainly needs to be removed from PyCBC

See also #4821.

Contents

This PR removes the pycbc_coinc_time script.

Links to any issues or associated PRs

Testing performed

Additional notes

  • The author of this pull request confirms they will adhere to the code of conduct

@GarethCabournDavies
Copy link
Contributor

This looks good to me, I don't think pycbc_coinc_time is used at all either (adding to Ian's point)

Can dqsegdb be removed from requirements-igwn.txt as well then?

@duncanmmacleod
Copy link
Member Author

Can dqsegdb be removed from requirements-igwn.txt as well then?

@GarethCabournDavies (@spxiwh) the only other references to the dqsegdb client are in various old workflow .ini files, and associated documentation .rst files, e.g:

$ git grep _dqsegdb
docs/hwinj.rst:  ligolw_segment_query_dqsegdb --query-segments --segment-url https://dqsegdb5.phy.syr.edu --gps-start-time 1124380361 --gps-end-time 1124382409 --include-segments L1:DMT-ANALYSIS_READY:1 --output-file L1-SEGMENTS.xml
docs/hwinj.rst:  ligolw_segment_query_dqsegdb --query-segments --segment-url https://dqsegdb5.phy.syr.edu --gps-start-time 1124380361 --gps-end-time 1124382409 --include-segments H1:DMT-ANALYSIS_READY:1 --output-file H1-SEGMENTS.xml
docs/resources/er6_bns.ini:segment_query = ${which:ligolw_segment_query_dqsegdb}
docs/resources/er6_bns.ini:segments_from_cats = ${which:ligolw_segments_from_cats_dqsegdb}
<snip>

However, I can't see any actual support for segment_query and segments_from_cats configuration options in the workflow libraries.

So, tentatively, I think that dqsegdb can be removed, but I am not an authority on this.

@GarethCabournDavies
Copy link
Contributor

It should be able to be removed. I also checked git grep "dqsegdb" | grep -v ligolw_segment

@titodalcanton
Copy link
Contributor

Could we just drop the dqsegdb dependency without necessarily removing this script?

@duncanmmacleod
Copy link
Member Author

Could we just drop the dqsegdb dependency without necessarily removing this script?

@titodalcanton are you suggesting that someone update the script to operate without dqsegdb, or leave the script broken in the project?

this is no longer required by any modules, scripts, or actively supported workflow configuration
@duncanmmacleod
Copy link
Member Author

It should be able to be removed. I also checked git grep "dqsegdb" | grep -v ligolw_segment

I have updated this PR to remove the requirement.

@spxiwh spxiwh merged commit ab7cf0c into gwastro:master Jul 27, 2024
32 checks passed
@duncanmmacleod duncanmmacleod deleted the remove-pycbc_coinc_time branch July 30, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants