Skip to content

Conversation

@egouden
Copy link
Contributor

@egouden egouden commented Oct 2, 2025

This is useful for stacking single sweeps or creating a new volume logic based on time.

@codecov
Copy link

codecov bot commented Oct 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.57%. Comparing base (727434c) to head (71bef80).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #307      +/-   ##
==========================================
+ Coverage   93.50%   93.57%   +0.07%     
==========================================
  Files          27       27              
  Lines        5587     5635      +48     
==========================================
+ Hits         5224     5273      +49     
+ Misses        363      362       -1     
Flag Coverage Δ
notebooktests 0.00% <0.00%> (ø)
unittests 93.57% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@egouden
Copy link
Contributor Author

egouden commented Oct 9, 2025

I added the option to select sweeps by angle. This is useful if you want to discard a calibration scan (i.e. 90 degree).

@egouden
Copy link
Contributor Author

egouden commented Oct 9, 2025

@kmuehlbauer

I found inconsistencies in the global variable sweep_group_name. Should it be a list of strings or ints? Most readers return ints but cfradial1 reader returns strings. There is also a conversion in the global metadata constructor.

I think the string is better because it is consistent with the variable name and you can use it directly for accessing the sweeps. I guess this has no negative impact on the performance.

@egouden egouden force-pushed the create_volume branch 2 times, most recently from 4c71b4b to 41ab76e Compare October 13, 2025 10:09
@egouden
Copy link
Contributor Author

egouden commented Oct 13, 2025

np.datetime64 is now used internally to avoid local time issues

@egouden egouden force-pushed the create_volume branch 2 times, most recently from 5fcb5c4 to 4ebd2a6 Compare October 14, 2025 10:42
Copy link
Collaborator

@kmuehlbauer kmuehlbauer left a comment

Choose a reason for hiding this comment

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

@egouden This is a nice addition. I've added a couple of comments.

@egouden egouden marked this pull request as draft October 24, 2025 14:09
@egouden egouden marked this pull request as ready for review October 24, 2025 14:59
xradar/util.py Outdated
Comment on lines 747 to 748
def format_zulu(t: np.datetime64) -> str:
return str(t).split(".")[0] + "Z"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@egouden This will strip any fractions of seconds. I've checked with Manual on Codes, Volume I.2 – International Codes, Annex II to the WMO Technical Regulations, Part B – Binary Codes, Part C – Common Features to Binary and Alphanumeric Codes, WMO-No. 306 and found that time_coverage_start/time_coverage_end is somehow ill-defined (string where it should be double).

Variable path/name Dimensions Type Comment
/time_coverage_start string UTC time of first ray in file

but

Variable path/name Dimensions Type Comment
/sweep_/time (time) double Coordinate variable for the time dimension. Each value is the time at centre of each ray.

Both have mandatory attributes as:

Variable path/name Attribute Kind Value
/sweep_/time units string “seconds since ” where is an ISO8601 time string of the form YYYY-MM-DDThh:mm:ssZ
calendar string See CF Conventions Appendix A
standard_name string “time”

So this seems like a flaw in the standard. Not sure how to go from here.

To get this functionality in (and as we still have proper sweep-times), would you mind adding a note on this in the code, so that we do not forget about it, @egouden? Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've contacted WMO to sort this out. Coming back, if there is some update.

@egouden egouden marked this pull request as draft December 10, 2025 10:47
@egouden egouden marked this pull request as ready for review December 10, 2025 11:14
@kmuehlbauer
Copy link
Collaborator

@egouden Please add an entry to history.md. After that, it's good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants