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

Select traces in Stream based on an Inventory #2531

Merged
merged 2 commits into from Jan 24, 2020

Conversation

claudiodsf
Copy link
Member

@claudiodsf claudiodsf commented Jan 12, 2020

What does this PR do?

Select traces in Stream based on the content of an Inventory object:
trace will be selected if the inventory contains a matching channel
active at the trace start time.

Why was it initiated? Any relevant Issues?

This is a follow-up of #2515.
Sometimes datacenter provides preassembled datasets with data and metadata for all the stations in the network (ex.: http://cnt.rm.ingv.it/event/23558121 --> Download).
Using #2515, inventory can be filtered based on geographic parameters.
With the present PR, data in stream can be selected based on the inventory content.

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).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+DOCS"
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .

Select traces in Stream based on the content of an Inventory object:
trace will be selected if the inventory contains a matching channel
active at the trace start time.
traces.append(trace)
except ValueError:
break
return self.__class__(traces=traces)
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly any other options specified to select() will be ignored if an Inventory is provided?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. It seems to me a strange use case to mix the two criteria. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should stick with the current approach of chaining all checks with logical AND, i.e. any filter criteria specified by the user should actually also be applied. I don't think it's good practice to have one kwarg completely override others too much, it'd be hell for a user to check which combination of kwargs together actually do what.

Copy link
Member

Choose a reason for hiding this comment

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

I shared @megies opinion. This would be really confusing to me and one passes multiple criteria I would intuitively expect them all to be applied.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll modify that.

Copy link
Member

Choose a reason for hiding this comment

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

Please try to get it done ASAP so I can bundle the RC thanks! :-)

Copy link
Member

Choose a reason for hiding this comment

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

Did it myself. Tests aren't comprehensive, btw, no fails popping up after my change it seems

@megies megies added .core issues affecting our functionality at the very core .core.inventory issues related to our Inventory functionality labels Jan 13, 2020
@megies megies added this to the 1.2.0 milestone Jan 13, 2020
@megies megies added this to In Progress in Release 1.2.0 Jan 13, 2020
@megies megies merged commit 3c17fa5 into obspy:master Jan 24, 2020
@megies megies deleted the stream_select_from_inventory branch January 24, 2020 12:39
@megies megies moved this from In Progress to Done in Release 1.2.0 Jan 24, 2020
@claudiodsf
Copy link
Member Author

@megies Thanks for taking care of this: I have been sick and unable to work during the last few days.

claudiodsf added a commit to claudiodsf/obspy that referenced this pull request Feb 26, 2020
megies added a commit that referenced this pull request Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.core.inventory issues related to our Inventory functionality .core issues affecting our functionality at the very core
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants