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

Fix issue 153 #154

Merged
merged 15 commits into from Jun 21, 2023
Merged

Fix issue 153 #154

merged 15 commits into from Jun 21, 2023

Conversation

kkappler
Copy link
Collaborator

@kkappler kkappler commented Jun 17, 2023

While fixing this, may as well fix issue #147 (its probably related)
Also, we should

  • add coverage of mth5.clients.fdsn
    • The len(run_list) == num_streams is covered by make_mth5_dirver
    • len(run_list) == 1 is needed (could maybe make PKD test do this in aurora)
    • len(run_list) != num_streams: Not sure
  • Simplify the logic in FDSN

kujaku11 and others added 3 commits June 8, 2023 14:52
- Factor some complexity out of make_mth5_from_fdsn_client
- factor (duplicated code) into stream_boundaries

[Issue(s): #156]
@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2023

Codecov Report

Patch coverage: 12.24% and project coverage change: +0.06 🎉

Comparison is base (52446f2) 59.73% compared to head (ff97100) 59.79%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@                Coverage Diff                @@
##           fix_issue_147     #154      +/-   ##
=================================================
+ Coverage          59.73%   59.79%   +0.06%     
=================================================
  Files                128      128              
  Lines              13019    13022       +3     
=================================================
+ Hits                7777     7787      +10     
+ Misses              5242     5235       -7     
Impacted Files Coverage Δ
mth5/clients/fdsn.py 12.55% <11.90%> (+4.40%) ⬆️
mth5/io/zen/coil_response.py 17.89% <14.28%> (-1.62%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Six instances of the same block of three lines replaced with one call.

[Issue(s): #153]
Also, in loop where there is only 1 run in runlist, but
multiple streams from FDSN, cast start,end from times[0],[1]
so that the rest of the logic is same as other cases

[Issue(s): #153]
Clean up logic for case n_runs==1
We were checking n_times > 1, with an elif n_times == 1
BUT
this logic lay within an elif following a check that n_runs==n_times

Since to be in this block at all, means that n_runs!=n_times,
then if n_runs==1 we know that n_times!=1, so we don't need to check/handle that

[Issue(s): #153]
This method is key to merging wrangle_runs_into_containers_v1 and
wrangle_runs_into_containers_v2, because a v1 mth5 object can get
a survey group with the same method as can a v2 survey_group

Thus we can replace:
  run_group = m.stations_group.get_station(station_id).add_run(run_id)
  &
  run_group = survey_group.stations_group.get_station(station_id).add_run(run_id)

with:
  run_group = mth5_obj_or_survey.stations_group.get_station(station_id).add_run(run_id)

[Issue(s): #153]
- cast start_times and end_times to UTCDateTime in stream_boundaries, as they are only accessed in that form later
- add run_timings_match_stream_timing method to greatly simplify len(run_list) != n_times case logic.

[Issue(s): #153]
replaced with get_station_streams method

[Issue(s): #153
also simplify interact logic at end of make

[Issue(s): #153]
Remove cruft.

[Issue(s): #153]
@kkappler
Copy link
Collaborator Author

This is ready to merge - if possible adding test coverage would be nice, but not required.

@kkappler kkappler requested a review from kujaku11 June 19, 2023 20:36
Copy link
Owner

@kujaku11 kujaku11 left a comment

Choose a reason for hiding this comment

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

Looks like a good refactor. Not sure about the one test that fails. Looking into it.

@kujaku11 kujaku11 merged commit c42b69f into fix_issue_147 Jun 21, 2023
6 of 12 checks passed
@kujaku11 kujaku11 deleted the fix_issue_153 branch June 21, 2023 18:29
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

3 participants