niosh-mining / obsplus

A Pandas-Centric ObsPy Expansion Pack
GNU Lesser General Public License v3.0
38 stars 8 forks source link

Client option to get response in df_to_inventory #182

Closed d-chambers closed 4 years ago

d-chambers commented 4 years ago

This PR adds the option to use a get_stations_kwargs row in a dataframe passed to obsplus.utils.stations.df_to_inventory in order to download an existing channels client with the default ObsPy fdsn client or one provided to the function.

For context, it is nice to keep a simple csv of channels and be able to edit it and regenerate an inventory rather than work with the highly nested inventory object directly. This adds just one more optional column that should make it easier to get the correct response for channels registered with an FDSN provider.

d-chambers commented 4 years ago

@sboltz mind taking a look at this? I know df_to_inventory is a but weird with so many nested functions but otherwise should be straightforward.

sboltz commented 4 years ago

Sure, I can take a look. Thanks for implementing it.

On May 28, 2020, at 18:21, Derrick notifications@github.com wrote:

 @sboltz mind taking a look at this? I know df_to_inventory is a but weird with so many nested functions but otherwise should be straightforward.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

codecov-commenter commented 4 years ago

Codecov Report

Merging #182 into master will decrease coverage by 0.08%. The diff coverage is 97.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
- Coverage   97.32%   97.23%   -0.09%     
==========================================
  Files          37       37              
  Lines        4069     4159      +90     
  Branches      647      661      +14     
==========================================
+ Hits         3960     4044      +84     
- Misses         52       54       +2     
- Partials       57       61       +4     
Flag Coverage Δ
#unittests 97.23% <97.90%> (-0.09%) :arrow_down:
Impacted Files Coverage Δ
obsplus/utils/stations.py 96.66% <97.41%> (-3.34%) :arrow_down:
obsplus/constants.py 100.00% <100.00%> (ø)
obsplus/exceptions.py 100.00% <100.00%> (ø)
obsplus/utils/docs.py 100.00% <100.00%> (ø)
obsplus/utils/misc.py 98.35% <100.00%> (+0.08%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d3d31d6...23968c1. Read the comment docs.

d-chambers commented 4 years ago

No problem, it seemed like the right thing to do.

d-chambers commented 4 years ago

Thanks @sboltz, all good points. I think I addressed all of them and I also couldn't resist the urge to refactor a little more. I will give it one more look-over tomorrow.

d-chambers commented 4 years ago

I should note this has one small breaking change: For the datalogger_keys and sensor_keys columns, obsplus now expects json strings or python objects. Separating the keys with __ was kinda weird, and a mistake in my opinion.

sboltz commented 4 years ago

I should note this has one small breaking change: For the datalogger_keys and sensor_keys columns, obsplus now expects json strings or python objects. Separating the keys with __ was kinda weird, and a mistake in my opinion.

I agree that that was a kind of weird way to do it (though convenient when you are too lazy to figure out the 'right' way to do it). I think taking that out makes sense.

d-chambers commented 4 years ago

In the face of ambiguity, refuse the temptation to guess.

I succumbed to the temperation to guess. It should probably raise an error if both NRL and station client are specified.