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.

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

Inventory copy #2322

Merged
merged 6 commits into from Feb 15, 2019
Merged

Inventory copy #2322

merged 6 commits into from Feb 15, 2019

Conversation

d-chambers
Copy link
Member

@d-chambers d-chambers commented Feb 14, 2019 •
edited by megies

What does this PR do?

Adds a copy method to Inventory to be consistent with Catalog and Stream which both have copy methods that created deep copies.

Why was it initiated? Any relevant Issues?

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"
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • 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 .
  • First time contributors have added your name to CONTRIBUTORS.txt .

@d-chambers d-chambers added this to the 1.2.0 milestone Feb 14, 2019
@d-chambers d-chambers added the .core.inventory issues related to our Inventory functionality label Feb 14, 2019
@krischer krischer added this to In Progress in Release 1.2.0 Feb 14, 2019
@krischer krischer removed this from In Progress in Release 1.2.0 Feb 14, 2019
@krischer krischer added this to In Progress in Release 1.2.0 Feb 14, 2019
@d-chambers d-chambers moved this from In Progress to Waiting for Review in Release 1.2.0 Feb 14, 2019
@megies
Copy link
Member

megies commented Feb 14, 2019

Rebased on current master and force-pushed so that we have fresh CI results tomorrow.

ThomasLecocq
ThomasLecocq previously requested changes Feb 15, 2019
View reviewed changes
obspy/core/inventory/inventory.py Outdated

>>> from obspy import read_inventory
>>> inv = read_inventory()
>>> inv2 = inv2.copy()
Copy link
Contributor

Choose a reason for hiding this comment

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

inv2 = inv.copy()

obspy/core/tests/test_inventory.py Outdated
self.assertEqual(inv, inv2)
# make sure changing inv2 doesnt affect inv
original_latitude = inv2[0][0][0].latitude
inv2[0][0][0].latitudue = original_latitude + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

latitudue - latitude

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure how to understand this test...

Copy link
Contributor

Choose a reason for hiding this comment

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

It just checks whether the memory copy works by modifying the copied reference and checking it doesn't touch the original reference.

Copy link
Member

Choose a reason for hiding this comment

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

um.. but the latitude is never actually changed anywhere.. so i agree with @ThomasLecocq

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the typo has to be fixed.. but the point is to prove that the two objects don't reference the same point in memory

Copy link
Member

Choose a reason for hiding this comment

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

Fixed the typo already and also amended the test

@ThomasLecocq ThomasLecocq mentioned this pull request Feb 15, 2019
Merged
8 tasks
megies
megies approved these changes Feb 15, 2019
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.

Looks good to me, can be merged on green CI

@megies megies moved this from Waiting for Review to Waiting on CI in Release 1.2.0 Feb 15, 2019
@megies
Copy link
Member

megies commented Feb 15, 2019

CI all green so far, only 1-2 appveyor jobs not yet done

@megies megies dismissed ThomasLecocq’s stale review February 15, 2019 16:57

proposed changes implemented

@ThomasLecocq
Copy link
Contributor

@megies all green (except unrelated evalresp)

@ThomasLecocq ThomasLecocq moved this from Waiting on CI to Waiting for final manual validation by Core Dev in Release 1.2.0 Feb 15, 2019
@megies megies merged commit c465fef into master Feb 15, 2019
@megies
Copy link
Member

megies commented Feb 15, 2019

Bääääm! 🚀

@megies megies deleted the inventory_copy branch February 15, 2019 17:41
@ThomasLecocq ThomasLecocq moved this from Waiting for final manual validation by Core Dev to Done in Release 1.2.0 Feb 15, 2019
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

@Jollyfant Jollyfant Jollyfant left review comments

@ThomasLecocq ThomasLecocq ThomasLecocq left review comments

Assignees
No one assigned
Labels
.core.inventory issues related to our Inventory functionality
Projects
No open projects
Release 1.2.0
Done
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@d-chambers @megies @ThomasLecocq @Jollyfant