Skip to content

Allow reading multiple blockette 55 entries (FAP) in dataless seed file #3277

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 16, 2023
Merged

Allow reading multiple blockette 55 entries (FAP) in dataless seed file #3277

merged 4 commits into from
Mar 16, 2023

Conversation

isbreen
Copy link
Contributor

@isbreen isbreen commented Mar 7, 2023 •
edited by megies
Loading

What does this PR do?

Allows reading multiple blockette 55 entries (FAP respons list) in dataless seed file.

Why was it initiated? Any relevant Issues?

fixes #3275

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.
  • 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 ready for review tag when you are ready for the PR to be reviewed.

@isbreen
Copy link
Contributor Author

isbreen commented Mar 7, 2023

Some tests for TauP are failing but this is probably not related to my changes. Making it ready for review now anyway.

@isbreen isbreen marked this pull request as ready for review March 7, 2023 14:20
@megies
Copy link
Member

megies commented Mar 13, 2023

@isbreen there are some file changes in taup data files in your PR. These need to be fixed.
This will also need a new test case. I'm not sure if this fits in maintenance branch, probably master is the better place for this, but that's not important right now, can be easily rebased later.

@megies megies added this to the 1.5.0 milestone Mar 13, 2023
@isbreen
Copy link
Contributor Author

isbreen commented Mar 13, 2023

@isbreen there are some file changes in taup data files in your PR. These need to be fixed. This will also need a new test case. I'm not sure if this fits in maintenance branch, probably master is the better place for this, but that's not important right now, can be easily rebased later.

Not sure where these taup changes came from. I didn't touch them. So not sure how to fix this. Will try.

@isbreen isbreen force-pushed the fix_seed_fap_resp branch from eb5d924 to c0be61e Compare March 14, 2023 09:25
@isbreen
Copy link
Contributor Author

isbreen commented Mar 14, 2023 •
edited
Loading

@megies I have added a test but I'm still struggling with these taup file changes. There have NOT been changed (I compared them with previous version with diff command). I tried with my limited git experience to uncommit these changes and then marked these as assumed unchanged but no luck. Any idea what is going on here and how I can fix this?

@isbreen isbreen force-pushed the fix_seed_fap_resp branch from eb48b04 to dfe2b96 Compare March 14, 2023 10:30
@isbreen isbreen changed the title Allow reading multiple blockette 33 entries (FAP) in dataless seed file Allow reading multiple blockette 55 entries (FAP) in dataless seed file Mar 14, 2023
@megies megies removed request for crotwell and johnrudge March 14, 2023 15:57
@megies
Copy link
Member

megies commented Mar 14, 2023

Sometimes on other platforms weird stuff happens that adds like a line ending on the last line of files (there's differences between nix and windows there) and if you commit everything that just goes in there. That's why I always recommend git add -p which asks you for each part of the diff.

Don't worry about it now, I can fix that later. Can you change the test to use the smaller file I posted in the issue, though?

@isbreen
Copy link
Contributor Author

isbreen commented Mar 15, 2023

Sometimes on other platforms weird stuff happens that adds like a line ending on the last line of files (there's differences between nix and windows there) and if you commit everything that just goes in there. That's why I always recommend git add -p which asks you for each part of the diff.

Ok, thanks for fixing this!

Don't worry about it now, I can fix that later. Can you change the test to use the smaller file I posted in the issue, though?

I have exchanged the test data files with your files. Thanks for editing the dataless-seeds! That was out of my expertise.

megies
megies reviewed Mar 16, 2023
View reviewed changes
obspy/io/xseed/tests/test_parser.py Outdated
inv_xml = obspy.read_inventory('issue3275.xml')
inv_seed = obspy.read_inventory('issue3275.seed')
inv_xml = obspy.read_inventory(testdata['issue3275.xml'])
inv_seed = obspy.read_inventory(testdata['issue3275.seed'])
Copy link
Member

Choose a reason for hiding this comment

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

@isbreen this is how to lookup test data within one module through the pytest fixture we provide, just fyi

@megies megies force-pushed the fix_seed_fap_resp branch from bd6e8bd to 372bc98 Compare March 16, 2023 09:32
megies
megies approved these changes Mar 16, 2023
View reviewed changes
Copy link
Member

@megies megies left a comment

Choose a reason for hiding this comment

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

Did some cleanup and rebasing/squashing commits to clean up history. Should be good to go if CI goes green

@isbreen
Copy link
Contributor Author

isbreen commented Mar 16, 2023

Did some cleanup and rebasing/squashing commits to clean up history. Should be good to go if CI goes green

Thanks!

@megies megies merged commit 0cb6e01 into obspy:maintenance_1.4.x Mar 16, 2023
@megies megies modified the milestones: 1.5.0, 1.4.1 Mar 16, 2023
@isbreen isbreen deleted the fix_seed_fap_resp branch March 16, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@megies megies megies approved these changes

Assignees
No one assigned
Labels
.io.xseed
Projects
None yet
Milestone
1.4.1
Development

Successfully merging this pull request may close these issues.
2 participants
@isbreen @megies