-
Notifications
You must be signed in to change notification settings - Fork 2
Feature/thickness aware notches #80
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
e62730f
c94afc3
2f57462
698606f
63cd892
448cf77
ec637c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| # import materials | ||
| from steer_opencell_design.Materials.Other import CurrentCollectorMaterial | ||
|
|
||
| from collections.abc import Iterable | ||
| from typing import Tuple, Optional | ||
| import numpy as np | ||
|
|
||
|
|
@@ -118,6 +119,7 @@ def __init__( | |
| insulation_width: Optional[float] = 0, | ||
| name: Optional[str] = "Notched Current Collector", | ||
| datum: Optional[Tuple[float, float, float]] = (0, 0, 0), | ||
| tab_center_positions: Optional[Iterable[float]] = None, | ||
| ) -> None: | ||
| """ | ||
| Initialize an object that represents a notched current collector. | ||
|
|
@@ -138,6 +140,10 @@ def __init__( | |
| Spacing between the tabs in mm. | ||
| tab_height : float | ||
| Height of the tabs in mm. | ||
| tab_center_positions : iterable of float, optional | ||
| Explicit tab center positions measured from the leading edge of the | ||
| foil in mm. When provided, these thickness-aware or otherwise | ||
| custom positions take precedence over ``tab_spacing``. | ||
| coated_tab_height : float | ||
| Height of the coated tab on the top side in mm. | ||
| bare_lengths_a_side : Tuple[float, float] | ||
|
|
@@ -151,6 +157,10 @@ def __init__( | |
| datum : Optional[Tuple[float, float, float]], default=(0, 0, 0) | ||
| Datum of the current collector in mm. | ||
| """ | ||
| # Must exist before the base-class initialization invokes coordinate | ||
| # hooks through this class's MRO. | ||
| self._tab_center_positions = None | ||
|
|
||
| super().__init__( | ||
| material=material, | ||
| x_foil_length=length, | ||
|
|
@@ -167,6 +177,7 @@ def __init__( | |
| ) | ||
|
|
||
| self.tab_spacing = tab_spacing | ||
| self.tab_center_positions = tab_center_positions | ||
| self._calculate_all_properties() | ||
| self._update_properties = True | ||
|
|
||
|
|
@@ -245,6 +256,19 @@ def _calculate_tab_positions(self) -> None: | |
| 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 | ||
|
|
||
| explicit_centers = getattr(self, "_tab_center_positions", None) | ||
| if explicit_centers is not None: | ||
| self._validate_tab_center_positions(explicit_centers) | ||
| centers = x_min + explicit_centers | ||
| self._tab_positions = np.column_stack( | ||
| ( | ||
| centers - self._tab_width / 2, | ||
| centers + self._tab_width / 2, | ||
| ) | ||
| ) | ||
| return | ||
|
|
||
| x_max = self._datum[0] + self._x_foil_length / 2 + self._tab_spacing | ||
|
|
||
| number_of_tabs = 1 | ||
|
|
@@ -272,6 +296,28 @@ def _calculate_tab_positions(self) -> None: | |
|
|
||
| self._tab_positions = np.column_stack((tab_starts, tab_ends)) | ||
|
|
||
| def _validate_tab_center_positions(self, positions: np.ndarray) -> None: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| """Validate explicit tab centers expressed in internal meter units.""" | ||
| if positions.ndim != 1: | ||
| raise ValueError("tab_center_positions must be a one-dimensional sequence.") | ||
| if not np.all(np.isfinite(positions)): | ||
| raise ValueError("tab_center_positions must contain only finite values.") | ||
| if len(positions) == 0: | ||
| return | ||
|
|
||
| minimum_center = self._tab_width / 2 | ||
| maximum_center = self._x_foil_length - self._tab_width / 2 | ||
| if positions[0] < minimum_center or positions[-1] > maximum_center: | ||
| raise ValueError( | ||
| "Each tab center must keep the full tab within the foil length." | ||
| ) | ||
|
|
||
| pitches = np.diff(positions) | ||
| if np.any(pitches <= 0): | ||
| raise ValueError("tab_center_positions must be strictly increasing.") | ||
| if np.any(pitches < self._tab_width): | ||
| raise ValueError("Explicit tabs cannot overlap.") | ||
|
|
||
| def _calculate_coordinates(self): | ||
| self._calculate_tab_positions() | ||
| super()._calculate_coordinates() | ||
|
|
@@ -378,11 +424,11 @@ def _get_insulation_coordinates(self, side: str = "a") -> np.ndarray: | |
|
|
||
| # Compute x bounds of coated region | ||
| bare_left, bare_right = self._bare_lengths_a_side if side == "a" else self._bare_lengths_b_side | ||
|
|
||
| # Check if bare lengths exceed foil length - return empty arrays if so | ||
| if bare_left + bare_right >= self._x_foil_length: | ||
| return np.empty((0, 3)) | ||
|
|
||
| x_start = self._datum[0] - self._x_foil_length / 2 + bare_left | ||
| x_end = self._datum[0] + self._x_foil_length / 2 - bare_right | ||
|
|
||
|
|
@@ -448,7 +494,7 @@ def _get_insulation_coordinates(self, side: str = "a") -> np.ndarray: | |
|
|
||
| # Create z array with proper numeric dtype | ||
| z = np.full_like(x, z_val, dtype=float) | ||
|
|
||
| # Handle None values by converting to NaN for numeric arrays | ||
| none_mask = np.array([val is None for val in x]) | ||
| if np.any(none_mask): | ||
|
|
@@ -461,6 +507,44 @@ def _get_insulation_coordinates(self, side: str = "a") -> np.ndarray: | |
| def tab_positions(self) -> list: | ||
| return [(start * M_TO_MM, end * M_TO_MM) for start, end in self._tab_positions] | ||
|
|
||
| @property | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice! |
||
| def tab_center_positions(self) -> Optional[list]: | ||
| """Return configured explicit centers from the foil leading edge in mm.""" | ||
| positions = getattr(self, "_tab_center_positions", None) | ||
| if positions is None: | ||
| return None | ||
| return (positions * M_TO_MM).tolist() | ||
|
|
||
| @property | ||
| def calculated_tab_center_positions(self) -> list: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice! |
||
| """Return all calculated tab centers in the collector coordinate system.""" | ||
| if len(self._tab_positions) == 0: | ||
| return [] | ||
| centers = self._tab_positions.mean(axis=1) | ||
| return (centers * M_TO_MM).tolist() | ||
|
|
||
| @property | ||
| def tab_pitches(self) -> list: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I will rename it. |
||
| """Return consecutive center-to-center pitches in mm.""" | ||
| if len(self._tab_positions) < 2: | ||
| return [] | ||
| centers = self._tab_positions.mean(axis=1) | ||
| return (np.diff(centers) * M_TO_MM).tolist() | ||
|
|
||
| @property | ||
| def tab_gaps(self) -> list: | ||
| """Return consecutive edge-to-edge notch gaps in mm.""" | ||
| if len(self._tab_positions) < 2: | ||
| return [] | ||
| return ( | ||
| (self._tab_positions[1:, 0] - self._tab_positions[:-1, 1]) * M_TO_MM | ||
| ).tolist() | ||
|
|
||
| @property | ||
| def number_of_tabs(self) -> int: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can call this n_tabs() |
||
| """Return the number of complete tabs in the current pattern.""" | ||
| return len(self._tab_positions) | ||
|
|
||
| @property | ||
| def tab_spacing(self) -> float: | ||
| return self._tab_spacing * M_TO_MM | ||
|
|
@@ -508,12 +592,16 @@ def tab_width_range(self) -> Tuple[float, float]: | |
| @tab_spacing.setter | ||
| @calculate_all_properties | ||
| def tab_spacing(self, tab_spacing: float) -> None: | ||
|
|
||
| self.validate_positive_float(tab_spacing, "tab_spacing") | ||
|
|
||
| self._tab_spacing = float(tab_spacing) * MM_TO_M | ||
| self._tab_gap = self._tab_spacing - self._tab_width | ||
|
|
||
| # Explicit positions and scalar spacing are mutually exclusive modes. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. I am wondering if the notched current collector should have an enumerated variable which defines the notch spacing mode. This would mostly be internal, but it would control which properties can be set. you could have options. Any thoughts on this? (dont implement yet)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree an enum could become useful as more spacing strategies are added. For the current two modes, I would prefer deriving the mode from whether explicit centers are present rather than storing duplicated state. |
||
| if hasattr(self, "_tab_center_positions"): | ||
| self._tab_center_positions = None | ||
|
|
||
| if self._tab_gap < 0: | ||
| raise ValueError("Tab spacing cannot be less than the tab width.") | ||
|
|
||
|
|
@@ -539,4 +627,30 @@ def tab_gap(self, tab_gap: float) -> None: | |
| # Update internal values | ||
| self._tab_gap = tab_gap_m | ||
| self._tab_spacing = new_tab_spacing | ||
| self._tab_center_positions = None | ||
|
|
||
| @tab_center_positions.setter | ||
| @calculate_all_properties | ||
| def tab_center_positions( | ||
| self, tab_center_positions: Optional[Iterable[float]] | ||
| ) -> None: | ||
| """Set explicit centers from the foil leading edge, in millimeters.""" | ||
| if tab_center_positions is None: | ||
| self._tab_center_positions = None | ||
| return | ||
| if isinstance(tab_center_positions, (str, bytes)) or not isinstance( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should add a validator in steer_core ValidationMixin and then call that to do this step |
||
| tab_center_positions, Iterable | ||
| ): | ||
| raise TypeError( | ||
| "tab_center_positions must be an iterable of numbers or None." | ||
| ) | ||
|
|
||
| try: | ||
| positions = np.asarray(list(tab_center_positions), dtype=float) * MM_TO_M | ||
| except (TypeError, ValueError) as exc: | ||
| raise TypeError( | ||
| "tab_center_positions must be an iterable of numbers or None." | ||
| ) from exc | ||
|
|
||
| self._validate_tab_center_positions(positions) | ||
| self._tab_center_positions = positions | ||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.