Skip to content

add prodml pulse width units #531

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
Merged
d-chambers merged 2 commits into from
Sep 9, 2025
Merged

add prodml pulse width units #531

d-chambers merged 2 commits into from
Sep 9, 2025

Conversation

@d-chambers
Copy link
Contributor

@d-chambers d-chambers commented Sep 9, 2025 •
edited by coderabbitai bot
Loading

Description

Some prodml files define pulse width units as pulseWidthUnit and others as pulseWidthUnits. This simple PR just makes sure both cases are handled.

Checklist

I have (if applicable):

  • referenced the GitHub issue this PR closes.
  • documented the new feature with docstrings or appropriate doc page.
  • included a test. See testing guidelines.
  • your name has been added to the contributors page (docs/contributors.md).
  • added the "ready_for_review" tag once the PR is ready to be reviewed.

Summary by CodeRabbit

  • Bug Fixes
    • Improved compatibility when reading PRODML Acquisition data by recognizing additional variants of pulse-width, pulse-rate, and gauge-length unit attributes to avoid missing unit values and parsing inconsistencies.
  • Chores
    • Internal mapping updates to support broader PRODML files while keeping the public API unchanged.

@d-chambers d-chambers added the IO Work for reading/writing different formats label Sep 9, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 9, 2025 •
edited
Loading

Walkthrough

Updated internal ProdML attribute mappings to recognize additional variants: "PulseWidthUnit", "PulseRateUnits", and "GaugeLengthUnits" now map to internal fields pulse_width_units, pulse_rate_units, and gauge_length_units. No function signature, error handling, or public API changes.

Changes

Cohort / File(s) Summary of Changes
PRODML attribute mappings
dascore/io/prodml/utils.py		 Added mappings in _get_prodml_attrs to handle "PulseWidthUnit" → pulse_width_units, "PulseRateUnits" → pulse_rate_units, and "GaugeLengthUnits" → gauge_length_units, alongside existing variants. No other logic or API changes.

Possibly related PRs

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the core change of adding support for ProdML pulse width unit variants, is concise and focused on the main feature, and avoids extraneous details or noise.
Description Check ✅ Passed The description includes the required “## Description” section clearly outlining the problem being solved and a matching “## Checklist” section per the repository template, fulfilling the structural requirements even though checklist items remain to be completed.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0ed7b7b and e16a8bf.

📒 Files selected for processing (1)
  • dascore/io/prodml/utils.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • dascore/io/prodml/utils.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: test_code_min_deps (windows-latest, 3.13)
  • GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
  • GitHub Check: test_code_min_deps (windows-latest, 3.12)
  • GitHub Check: test_code (ubuntu-latest, 3.10)
  • GitHub Check: test_code (ubuntu-latest, 3.12)
  • GitHub Check: test_code (windows-latest, 3.10)
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch prodml_width_units

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Sep 9, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.88%. Comparing base (7139676) to head (e16a8bf).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #531   +/-   ##
=======================================
  Coverage   99.88%   99.88%           
=======================================
  Files         122      122           
  Lines       10035    10035           
=======================================
  Hits        10023    10023           
  Misses         12       12
Flag Coverage Δ
unittests 99.88% <ø> (ø)

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.

coderabbitai[bot]
coderabbitai bot reviewed Sep 9, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
dascore/io/prodml/utils.py (1)

103-119: Add GaugeLengthUnits alias for symmetry and resilience.

Some datasets mirror the pluralization pattern for gauge length too. Cheap guardrail to avoid a future bug.

         "GaugeLength": "gauge_length",
-        "GaugeLengthUnit": "gauge_length_units",
+        "GaugeLengthUnit": "gauge_length_units",
+        "GaugeLengthUnits": "gauge_length_units",
         "GaugeLength.uom": "gauge_length_units",
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7139676 and 0ed7b7b.

📒 Files selected for processing (1)
  • dascore/io/prodml/utils.py (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
  • GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
  • GitHub Check: test_code_min_deps (windows-latest, 3.12)
  • GitHub Check: test_code_min_deps (windows-latest, 3.13)
  • GitHub Check: test_code_min_deps (macos-latest, 3.13)
  • GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
  • GitHub Check: test_code (windows-latest, 3.10)
  • GitHub Check: test_code (macos-latest, 3.10)
  • GitHub Check: test_code (windows-latest, 3.12)
  • GitHub Check: test_code (ubuntu-latest, 3.13)
  • GitHub Check: test_code (macos-latest, 3.13)
  • GitHub Check: test_code (ubuntu-latest, 3.10)
  • GitHub Check: test_code (windows-latest, 3.13)
  • GitHub Check: test_code (macos-latest, 3.12)
  • GitHub Check: test_code (windows-latest, 3.11)
  • GitHub Check: test_code (ubuntu-latest, 3.12)
  • GitHub Check: test_code (macos-latest, 3.11)
  • GitHub Check: test_code (ubuntu-latest, 3.11)
🔇 Additional comments (2)
dascore/io/prodml/utils.py (2)

108-113: LGTM: Now covers both singular/plural unit keys for pulse width/rate.

Adding "PulseWidthUnit" and "PulseRateUnits" eliminates common PRODML schema variance. Looks good.

103-119: Add lowerCamelCase mappings or normalize keys
Search didn’t surface any lowercase-camel variants—please manually verify that PRODML attributes like pulseWidthUnit/pulseRateUnits/gaugeLengthUnits don’t appear in your files or tests. If they do, add those aliases alongside the existing PascalCase keys in _root_attrs (or normalize attribute names before lookup).

@d-chambers d-chambers merged commit 0151905 into master Sep 9, 2025
22 checks passed
@d-chambers d-chambers deleted the prodml_width_units branch September 9, 2025 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

Assignees

No one assigned

Labels

IO Work for reading/writing different formats

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@d-chambers