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

Extend 3c transforms #516

Merged
merged 8 commits into from
Feb 27, 2024
Merged

Extend 3c transforms #516

merged 8 commits into from
Feb 27, 2024

Conversation

pavlis
Copy link
Collaborator

@pavlis pavlis commented Feb 24, 2024

Fixes and extensions to address Issue #511. Five major changes/additions:

  1. Adds new transform_to_RTZ function
  2. Adds new transform_to_LQT function
  3. Changes the api for free_surface_transformation to add more flexible input. Default now assumes fetching required data from Metadata to make the function easier to use in a map operator.
  4. Complete pytest functions for the above.
  5. Cleaned up a confusion on the sign of the angle of rotation for the overloaded C++ function. That is now largely hidden behind the higher level functions anyway but the most important fix is making the sign convention unambiguous in the doxygen comments.

Note I'm pushing this to verify all the changes match my local test runs. I want to do a final sanity check against data before suggesting this really be merged.

@pavlis
Copy link
Collaborator Author

pavlis commented Feb 24, 2024

Oh yes, forgot to run the C test function. That should also adress issue #512.

@pavlis
Copy link
Collaborator Author

pavlis commented Feb 26, 2024

Assuming the current set of tests pass on github this branch is ready to merge. The pytest functions for the new set of Seismogram wrapper functions are fully tested, although we should verify if the coverage gizmo flags anything. I also tested them with my tutorial notebook on three component data transformations and I get the answers I expect. Hence, unless something unexpected pops up in the tests currently running I advise you, @wangyinz, to merge this branch with master.

@pavlis
Copy link
Collaborator Author

pavlis commented Feb 26, 2024

Well, obviously that last push didn't pass the pytest run. Rather mysterious, but my fault for not explicitly running the test under pytest and not checking the exception handler code. I forgot that that only could be tested under pytest.

Working on that so ignore this until you see it pass all these tests. Sorry, @wangyinz , for being overconfident this was ready to merge

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Attention: Patch coverage is 91.17647% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 55.24%. Comparing base (b45b04c) to head (70f8462).
Report is 4 commits behind head on master.

Files Patch % Lines
python/mspasspy/algorithms/basic.py 90.97% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
+ Coverage   55.08%   55.24%   +0.15%     
==========================================
  Files         144      144              
  Lines       22429    22560     +131     
==========================================
+ Hits        12356    12464     +108     
- Misses      10073    10096      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pavlis
Copy link
Collaborator Author

pavlis commented Feb 27, 2024

@wangyinz I think you should merge this branch. I noticed codecov flagged some missing testing for error handler lines that do kill if the rotate_to_standard throws an exception. I am 100% sure those handlers work. There is one other that issues a rare complaint message that is very unlikely to ever be executed. I'm sure the syntax is right and it won't fail if used. Adding a test for such a rare event seems unnecessary to me. If you disagree I'll try to create something.

@wangyinz wangyinz merged commit 9446d54 into master Feb 27, 2024
12 checks passed
@wangyinz wangyinz deleted the extend_3c_transforms branch February 27, 2024 16:07
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.

2 participants