Skip to content

Conversation

@marc-flex
Copy link
Contributor

@marc-flex marc-flex commented Dec 19, 2025

Greptile Summary

Added validation to DCVoltageSource that warns users when duplicate voltage values are detected in the voltage array. The implementation treats 0 and -0 as the same value by normalizing zeros before checking for duplicates.

  • Implemented check_repeated_voltage validator that normalizes zero values and uses np.unique() to detect duplicates
  • Added comprehensive test coverage including edge cases for zero normalization and multiple duplicates
  • Updated CHANGELOG.md with clear user-facing description of the new feature

Note: Previous thread comments identified a floating-point precision issue - the current implementation only normalizes zeros but uses direct equality for non-zero values via np.unique(), which may miss near-duplicate floating-point values like 1.0 and 1.0000000001.

Confidence Score: 4/5

  • This PR is safe to merge with minor style improvements recommended
  • The implementation is functionally correct for the intended use case of detecting exact duplicates and normalizing zeros. Previous thread already identified the floating-point precision consideration for non-zero values. Test coverage is comprehensive. Only minor style improvement suggested for message formatting.
  • Pay attention to tidy3d/components/spice/sources/dc.py regarding the floating-point comparison approach discussed in previous comments

Important Files Changed

Filename Overview
tidy3d/components/spice/sources/dc.py Added duplicate voltage validation with zero normalization. Previous thread identified floating-point precision issue that needs addressing.
tests/test_components/test_heat_charge.py Comprehensive test coverage for duplicate voltage warning, including edge cases like 0/-0 and multiple duplicates.
CHANGELOG.md Clear changelog entry in Added section describing the new validation feature.

Sequence Diagram

sequenceDiagram
    participant User
    participant DCVoltageSource
    participant Validator
    participant numpy as NumPy
    participant Logger

    User->>DCVoltageSource: Create with voltage array
    DCVoltageSource->>Validator: @validator("voltage") check_voltage
    Validator->>Validator: Check for infinite values
    Validator-->>DCVoltageSource: Return validated values
    
    DCVoltageSource->>Validator: @validator("voltage") check_repeated_voltage
    Validator->>numpy: np.isclose(val, 0, atol=1e-10)
    numpy-->>Validator: Boolean mask for zeros
    Validator->>numpy: np.where(mask, 0.0, val)
    numpy-->>Validator: Normalized array (zeros unified)
    Validator->>numpy: np.unique(normalized)
    numpy-->>Validator: Unique values array
    Validator->>Validator: Compare len(unique_values) < len(val)
    alt Duplicates found
        Validator->>Logger: log.warning(duplicate message)
        Logger-->>User: Display warning
    end
    Validator-->>DCVoltageSource: Return original values
    DCVoltageSource-->>User: Instance created
Loading

@marc-flex marc-flex self-assigned this Dec 19, 2025
@marc-flex marc-flex marked this pull request as ready for review December 19, 2025 14:35
Copilot AI review requested due to automatic review settings December 19, 2025 14:36
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds validation to the DCVoltageSource class to warn users when duplicate voltage values are present in the voltage array. The implementation correctly handles the special case where 0.0 and -0.0 are treated as the same value.

  • Added a new validator check_repeated_voltage that warns when duplicate voltage values are detected
  • Implemented special handling to normalize 0.0 and -0.0 as the same value before checking for duplicates
  • Added comprehensive test coverage for various duplicate scenarios

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
tidy3d/components/spice/sources/dc.py Added new validator method check_repeated_voltage with logic to detect and warn about duplicate voltage values, including special handling for zero values
tests/test_components/test_heat_charge.py Added test function test_repeated_voltage_warning with multiple test cases covering unique values, repeated values, 0/-0 duplicates, and multiple repeated values

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@marc-flex marc-flex force-pushed the feat/FXC-4519/duplicated_voltage_values branch from 48dc80e to 54840fd Compare December 19, 2025 14:53
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@marc-flex marc-flex force-pushed the feat/FXC-4519/duplicated_voltage_values branch from 54840fd to b93f677 Compare December 19, 2025 15:01
@marc-flex
Copy link
Contributor Author

@greptile update summary

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@flexcompute flexcompute deleted a comment from greptile-apps bot Dec 19, 2025
@marc-flex marc-flex force-pushed the feat/FXC-4519/duplicated_voltage_values branch from 008c617 to c81e649 Compare December 19, 2025 15:09
@github-actions
Copy link
Contributor

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/components/spice/sources/dc.py (100%)

Summary

  • Total: 9 lines
  • Missing: 0 lines
  • Coverage: 100%

"Duplicate voltage values detected in 'voltage' array. "
f"Found {len(val)} values but only {len(unique_values)} are unique. "
"Note: '0' and '-0' are considered the same value."
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally looks good, but the one thing that bugs me a little is there's no guaranteed correspondence between what's validated here and what values the solver will compute at. Would be nice to have a single source of truth for the "computed voltages", which also e.g. defines the tolerance within which two values are considered identical.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants