Skip to content

Feature/thickness aware notches - #80

Open
PTLin84 wants to merge 7 commits into
stanford-developers:mainfrom
PTLin84:feature/thickness-aware-notches
Open

Feature/thickness aware notches#80
PTLin84 wants to merge 7 commits into
stanford-developers:mainfrom
PTLin84:feature/thickness-aware-notches

Conversation

@PTLin84

@PTLin84 PTLin84 commented Aug 9, 2026

Copy link
Copy Markdown

Summary

This PR adds opt-in, thickness-aware notch alignment for FlatWoundJellyRoll.

Previously, NotchedCurrentCollector used constant center-to-center tab spacing. After winding, however, outer racetrack layers have longer perimeters, causing tabs to progressively drift out of alignment.

This change uses the calculated racetrack geometry to generate uneven tab positions along the unwrapped foil so that their centers align at the same racetrack phase after winding.

Below are example plots showing how tabs on cathode current collector align at the same phase after winding:
image
image
image

Design

The implementation follows the existing object responsibilities:

NotchedCurrentCollector

Adds support for explicit tab-center positions:

collector = NotchedCurrentCollector(
    ...,
    tab_center_positions=[50, 155, 270, 395],
)

These positions are measured from the foil's leading edge and provide the low-level representation for non-uniform tab patterns.

Explicit positions are validated for ordering, overlap, and foil-boundary constraints. Setting tab_spacing or tab_gap returns the collector to uniform-spacing mode.

SpiralCalculator

Adds aligned_positions_from_spiral(), which finds unwrapped foil positions satisfying:

theta(s_k) = alignment_phase + 2*pi*k

This ensures that generated tab centers share the same racetrack phase across successive wound layers while respecting tab-width and minimum-gap constraints.

FlatWoundJellyRoll

Adds independent cathode and anode alignment controls:

roll = FlatWoundJellyRoll(
    laminate=layup,
    mandrel=mandrel,
    cathode_notch_alignment_angle=0.0,
    anode_notch_alignment_angle=np.pi,
)

The jelly roll calculates the corresponding unwrapped tab-center positions from its wound geometry and applies them to the collectors.

Setting either alignment angle to None restores the existing uniform-spacing behavior for that electrode.

Alignment Angle

The alignment angle represents a normalized phase around the racetrack perimeter rather than a literal geometric angle everywhere on the shape.

A phase difference of corresponds to one complete trip around the racetrack, so:

alignment_phase + 2*pi*k

identifies the same normalized racetrack phase on successive wound layers.

Because outer layers have longer perimeters, the resulting tab spacing on the unwrapped foil increases with winding radius.

Public API

New NotchedCurrentCollector input:

  • tab_center_positions

New collector results:

  • calculated_tab_center_positions
  • tab_pitches
  • tab_gaps
  • number_of_tabs

New FlatWoundJellyRoll inputs:

  • cathode_notch_alignment_angle
  • anode_notch_alignment_angle

New output and visualization:

  • thickness_aware_notch_data
  • plot_notch_alignment()

Backward Compatibility

This feature is fully opt-in:

  • Existing NotchedCurrentCollector construction remains supported.
  • Constant tab_spacing remains the default.
  • Existing flat-wound assemblies behave unchanged when both alignment settings are None.
  • Explicit manual tab-center patterns remain available as a lower-level option.

Testing

Added coverage for explicit tab-center patterns, validation, serialization, same-phase alignment, boundary handling, independent cathode/anode alignment, disabling alignment, and visualization.

Full test suite:

951 passed, 30 dependency deprecation warnings
Coverage: 83.57%
conda run -n STEER pytest

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

All contributors have signed the CLA. Thank you!
Posted by the CLA Assistant Lite bot.

@PTLin84

PTLin84 commented Aug 9, 2026

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

github-actions Bot added a commit that referenced this pull request Aug 9, 2026

@nicholas9182 nicholas9182 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi Kyle - great work!

The implementation is very clean.

My main thought is - at the moment the tabs are defined by their racetrack angle. I think they should instead be defined (at least the public APIs) by two variables:
1) the distance along the racetrack centre line
2) whether they are arranged transverse or longitudinal
in this way they become similar to the punched current collector designs. A key visualisation will then be the top-down view of the jelly roll, and seeing where the tabs end up sticking out. We should also add a limit to the position of the tabs so they cant sit on the curved part of the racetrack

Function to calculate the positions of the tabs along the length of the current collector.
"""
x_min = self._datum[0] - self._x_foil_length / 2

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

add a code comment here to explain what this bit is doing

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

alternatively I would break this function out into two separate functions

def _calculate_explicit_tab_positions
def _calculate_regular_tab_positions

and then _calculate_tab_positions can just route to the right one depending

generally I am a fan of lots of small functions each with a dedicated purpose.


self._tab_positions = np.column_stack((tab_starts, tab_ends))

def _validate_tab_center_positions(self, positions: np.ndarray) -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

may be worth adding the word 'explicit' into the function name to convey that this is for when explicit tab positions are being used

def tab_positions(self) -> list:
return [(start * M_TO_MM, end * M_TO_MM) for start, end in self._tab_positions]

@property

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nice!

return (positions * M_TO_MM).tolist()

@property
def calculated_tab_center_positions(self) -> list:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nice!

return (centers * M_TO_MM).tolist()

@property
def tab_pitches(self) -> list:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am a little unsure what 'pitches' refers too. Is it the spacing between the tabs?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes. I will rename it.


# get the thickness maximum bound
big_layup = deepcopy(self._layup)
self._clear_generated_notches_on_layup(big_layup)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think these may be not-needed as the notch position shouldn't affect the jelly roll thickness. These are just used to get the upper and lower bound for the thickness value

return self._pressed_straight_length * M_TO_MM

@property
def cathode_notch_alignment_angle(self) -> Optional[float]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we can replace with cathode_notch_alignment_position


@cathode_notch_alignment_angle.setter
@calculate_all_properties
def cathode_notch_alignment_angle(self, value: Optional[float]) -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nice!

)

@property
def anode_notch_alignment_angle(self) -> Optional[float]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

usually angles are returned through properties im degrees (as the human readable unit). We can add the radians-degrees converter to steer-core if not there already

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think we're replacing the public angle API by a position in millimeters, so this becomes obsolete.

@anode_notch_alignment_angle.setter
@calculate_all_properties
def anode_notch_alignment_angle(self, value: Optional[float]) -> None:
self._anode_notch_alignment_angle = self._validate_notch_alignment_angle(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the convention generally used throughout if for validating functions to return None. This would then become

self._validate_notch_alignment_angle(value, "anode_notch_alignment_angle")
self._anode_notch_alignment_angle = value

check elsewhere if changes need to be made

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