-
Notifications
You must be signed in to change notification settings - Fork 12
fix: guard against empty peak array in get_ignition_delay #24
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
3ee50bb
fix: guard against empty peak array in get_ignition_delay
LekiaAnonim 8f65955
Compute mph from target_derivative for consistency with the other der…
LekiaAnonim ae9f7c4
Ignition delay was set to empty when peak_ind size is zero, this woul…
LekiaAnonim dcef174
Removed the python 3.5 and 3.6 classifiers from setup.py
LekiaAnonim 80209b4
Removed the example output artifact from commit
LekiaAnonim d37fc63
fix: handle nonpositive derivative peaks in ignition delay
LekiaAnonim 2338592
Potential fix for pull request finding
LekiaAnonim 8de32f0
ci: align supported Python range with dependency compatibility
LekiaAnonim 17b1e4e
Merge branch 'master' of https://github.com/LekiaAnonim/PyTeCK
LekiaAnonim File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| average deviation function: 7.6446209680420125 | ||
| average error function: 58.91886752410112 | ||
| datasets: | ||
| - absolute deviation: 7.6446209680420125 | ||
| datapoints: | ||
| - composition: | ||
| - {InChI: 1S/H2/h1H, amount: '0.00444', species-name: H2} | ||
| - {InChI: 1S/O2/c1-2, amount: '0.00556', species-name: O2} | ||
| - {InChI: 1S/Ar, amount: '0.99', species-name: Ar} | ||
| composition type: mole fraction | ||
| experimental ignition delay: 0.00047154 second | ||
| pressure: 220000.0 pascal | ||
| simulated ignition delay: 0.0010007455162881925 second | ||
| temperature: 1164.48 kelvin | ||
| - composition: | ||
| - {InChI: 1S/H2/h1H, amount: '0.00444', species-name: H2} | ||
| - {InChI: 1S/O2/c1-2, amount: '0.00556', species-name: O2} | ||
| - {InChI: 1S/Ar, amount: '0.99', species-name: Ar} | ||
| composition type: mole fraction | ||
| experimental ignition delay: 0.00044803 second | ||
| pressure: 220000.0 pascal | ||
| simulated ignition delay: 0.0009972083218715213 second | ||
| temperature: 1164.97 kelvin | ||
| - composition: | ||
| - {InChI: 1S/H2/h1H, amount: '0.00444', species-name: H2} | ||
| - {InChI: 1S/O2/c1-2, amount: '0.00556', species-name: O2} | ||
| - {InChI: 1S/Ar, amount: '0.99', species-name: Ar} | ||
| composition type: mole fraction | ||
| experimental ignition delay: 0.00029157 second | ||
| pressure: 220000.0 pascal | ||
| simulated ignition delay: 0.000574010524129157 second | ||
| temperature: 1264.2 kelvin | ||
| - composition: | ||
| - {InChI: 1S/H2/h1H, amount: '0.00444', species-name: H2} | ||
| - {InChI: 1S/O2/c1-2, amount: '0.00556', species-name: O2} | ||
| - {InChI: 1S/Ar, amount: '0.99', species-name: Ar} | ||
| composition type: mole fraction | ||
| experimental ignition delay: 0.00020593 second | ||
| pressure: 220000.0 pascal | ||
| simulated ignition delay: 0.00042129329629920055 second | ||
| temperature: 1332.57 kelvin | ||
| - composition: | ||
| - {InChI: 1S/H2/h1H, amount: '0.00444', species-name: H2} | ||
| - {InChI: 1S/O2/c1-2, amount: '0.00556', species-name: O2} | ||
| - {InChI: 1S/Ar, amount: '0.99', species-name: Ar} | ||
| composition type: mole fraction | ||
| experimental ignition delay: 8.811e-05 second | ||
| pressure: 220000.0 pascal | ||
| simulated ignition delay: 0.00021169358253286696 second | ||
| temperature: 1519.18 kelvin | ||
| dataset: H2O2Ar-shocktube-example.yaml | ||
| dataset_id: 0 | ||
| error function: 58.91886752410112 | ||
| standard deviation: 0.1 | ||
| error function standard deviation: 0.0 | ||
| model: h2o2.cti | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| ## Bug: `get_ignition_delay` crashes on empty peak array | ||
|
|
||
| ### Description | ||
|
|
||
| `get_ignition_delay()` in `pyteck/simulation.py` raises `ValueError: attempt to get argmax of an empty sequence` when `detect_peaks` returns no peaks. This happens when the input signal is flat or nearly flat (e.g., from a non-igniting reactor simulation). | ||
|
|
||
| All four ignition types are affected: `max`, `d/dt max`, `1/2 max`, and `d/dt max extrapolated`. | ||
|
|
||
| The function already has a safety check at the bottom: | ||
|
|
||
| ```python | ||
| if ign_delays.size == 0: | ||
| # ... warning + return np.array([0.0]) | ||
| ``` | ||
|
|
||
| But execution never reaches it because `peak_inds[np.argmax(target[peak_inds])]` crashes first when `peak_inds` is empty. | ||
|
|
||
| ### Minimal Reproducer | ||
|
|
||
| ```python | ||
| import numpy as np | ||
| from pyteck.simulation import get_ignition_delay | ||
|
|
||
| # Flat signal — no ignition occurred | ||
| time = np.linspace(0, 0.05, 100) | ||
| temperature = np.full_like(time, 500.0) | ||
|
|
||
| # Crashes with: ValueError: attempt to get argmax of an empty sequence | ||
| ign = get_ignition_delay(time, temperature, 'temperature', 'd/dt max') | ||
| ``` | ||
|
|
||
| ### Expected Behavior | ||
|
|
||
| The function should return `np.array([0.0])` (the existing "no ignition detected" sentinel), not crash. | ||
|
|
||
| ### Root Cause | ||
|
|
||
| In each branch (`max`, `d/dt max`, etc.), `detect_peaks` may return an empty array when the signal has no meaningful peaks. The line: | ||
|
|
||
| ```python | ||
| max_ind = peak_inds[np.argmax(target[peak_inds])] | ||
| ``` | ||
|
|
||
| then calls `np.argmax` on an empty slice, which raises `ValueError`. | ||
|
|
||
| ### Suggested Fix | ||
|
|
||
| Add an empty-array guard after each `detect_peaks` call so that execution falls through to the existing safety check: | ||
|
|
||
| ```python | ||
| peak_inds = detect_peaks(target, edge=None, mph=1.e-9*np.max(target)) | ||
|
|
||
| if peak_inds.size == 0: | ||
| ign_delays = np.array([]) | ||
| else: | ||
| max_ind = peak_inds[np.argmax(target[peak_inds])] | ||
| ign_delays = time[peak_inds[peak_inds <= max_ind]] | ||
| ``` | ||
|
|
||
| This needs to be applied in all four branches (`max`, `d/dt max`, `1/2 max`, `d/dt max extrapolated`). | ||
|
|
||
| ### Context | ||
|
|
||
| I'm using `get_ignition_delay` as a standalone function to post-process Cantera 0-D reactor output across a large parameter grid (temperature × pressure × equivalence ratio × multiple kinetic models). The grid intentionally includes conditions where ignition does not occur, which triggers this crash. | ||
|
|
||
| ### Environment | ||
|
|
||
| - PyTeCK development branch (commit with `get_ignition_delay` as a standalone function) | ||
| - NumPy 1.x | ||
| - Python 3.9 | ||
|
LekiaAnonim marked this conversation as resolved.
Outdated
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.