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

Aqms client #3398

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Aqms client #3398

wants to merge 2 commits into from

Conversation

mikehagerty
Copy link

@mikehagerty mikehagerty commented Jan 30, 2024

What does this PR do?

Please fill in

Why was it initiated? Any relevant Issues?

This branch was created to add a new aqms proxy waveserver (PWS) client

Please fill in (link relevant issues with "see #123456", mark issues that get resolved with "fixes #12345")

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes

  • This PR is not directly related to an existing issue (which has no PR yet).

  • While the PR is still work-in-progress, the no_ci label can be added to skip CI builds

  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just add the build_docs tag to this PR.
    Docs will be served at docs.obspy.org/pr/{branch_name} (do not use master branch).
    Please post a link to the relevant piece of documentation.

  • If all tests including network modules (e.g. clients.fdsn) should be tested for the PR,
    just add the test_network tag to this PR.

  • All tests still pass.

  • Any new features or fixed regressions are covered via new tests.
    **There's no way to do this as all of the PWS I know of are behind VPNs ! **

  • Any new or changed features are fully documented.

  • Significant changes have been added to CHANGELOG.txt .

  • First time contributors have added your name to CONTRIBUTORS.txt .

  • If the changes affect any plotting functions you have checked that the plots
    from all the CI builds look correct. Add the "upload_plots" tag so that plotting
    outputs are attached as artifacts.

  • New modules, add the module to CODEOWNERS with your github handle

  • Add the yellow ready for review label when you are ready for the PR to be reviewed.

@mikehagerty
Copy link
Author

Here are some things I had to fix (I made these changes for my own testing
but did not check them into my fork)

  1. setup.py - I had to manually set the version as the tag that get_git_version() gets
    is rejected by pip so that: >pip install -e fails
    This is true on both mac osx + linux

The next 2 issues were only for linux [ gcc version 4.8.5 20150623 (Red Hat 4.8.5-44) (GCC) ]

  1. On linux I get gcc errors on signal/src/aic_simple.c due to initial declarations in loop
    gcc -pthread -B /home/aqms/anaconda3/envs/test-mth/compiler_compat -Wno-unused-result -Wsign-compare -DNDEBUG -fwrapv -O2 -Wall -fPIC -O2 -isystem /home/aqms/anaconda3/envs/test-mth/include -fPIC -O2 -isystem /home/aqms/anaconda3/envs/test-mth/include -fPIC -I/home/aqms/anaconda3/envs/test-mth/include/python3.10 -c obspy/signal/src/aic_simple.c -o /tmp/tmpngtpkbiv.build-temp/obspy/signal/src/aic_simple.o
    obspy/signal/src/aic_simple.c: In function ‘aic_simple’:
    obspy/signal/src/aic_simple.c:40:9: error: ‘for’ loop initial declarations are only allowed in C99 mode
    for (int i = 0; i < size; i++) {
    ^
    obspy/signal/src/aic_simple.c:40:9: note: use option -std=c99 or -std=gnu99 to compile your code
    obspy/signal/src/aic_simple.c:40:27: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
    for (int i = 0; i < size; i++) {

I "fixed" this by adding -C99 flags in setup.py, e.g., under SIGNAL:
kwargs['extra_compile_args'] = ['-std=c99']

  1. However, this "fix" then causes a new problem, c99 doesn't have math.h so M_PI
    is undefined in signal/src/lanczos_resampling.c so I have to
    fix it there:
    signal/src/lanczos_resampling.c :
    /* MTH: -c99 doesn't have math.h hence no M_PI! /
    #ifndef M_PI
    #define M_PI (3.14159265358979323846264338327950288)
    #endif /
    M_PI */

I realize there is probably a better way to resolve 2. so that 3. doesn't occur ...
just thought I'd point out the steps I had to take to get this running on redhat.

Also, as this client connects to aqms proxy waveservers and the only ones I know
of are behind VPNs (so that I can't even connect to them from my own machine),
I didn't write any tests, etc.

@megies
Copy link
Member

megies commented Feb 27, 2024

@mikehagerty, I'm not really sure how to proceed here. All the issues there we could figure out, but adding a client for which we can't test because there is no servers to try is a bit of a weird one. Any chance there might be an open server to test against anytime soon?

@megies megies added this to the Future release milestone Feb 27, 2024
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.

None yet

2 participants