fix: guard against empty peak array in get_ignition_delay#24
Merged
Conversation
get_ignition_delay crashed with ValueError: attempt to get argmax of an empty sequence when detect_peaks returned no peaks (e.g. flat/non-igniting signal). Added peak_inds.size == 0 guards in all four ignition type branches so execution falls through to the existing sentinel return. Also relaxes upper version bounds on pyyaml/pint/numpy in setup.py and adds python_requires>=3.7.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a crash in get_ignition_delay when detect_peaks returns no peaks (e.g., flat/non-igniting signals), and updates packaging/example artifacts.
Changes:
- Add
peak_inds.size == 0guards in all ignition-type branches ofget_ignition_delayto avoidargmaxon an empty array. - Relax dependency upper bounds in
setup.pyand declarepython_requires>=3.7. - Update the H2O2 shock tube example invocation and add an example results YAML output file; add a markdown write-up of the crash scenario.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
pyteck/simulation.py |
Adds empty-peak guards to prevent ValueError when no peaks are detected. |
setup.py |
Removes dependency upper bounds and adds python_requires. |
examples/h2o2-shocktube/main.py |
Wraps the example execution in a __main__ guard and sets skip_validation=True. |
examples/h2o2-shocktube/h2o2-results.yaml |
Adds a committed example output artifact. |
pyteck_issue_get_ignition_delay_crash.md |
Documents the crash and reproducer/fix rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ivative-based branch
…d trigger the ign_delay.size==0 condition, that writes target-data-.out files, and could cause some noisy dumps. Instead, a no ignition delay of 0 array is return if peak_inds.size==0
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
setup.py:36
- Dependency upper bounds were removed here, but the repo’s conda environment specs still pin
numpy<2.0,pyyaml<4.0, andpint<0.9(see build-environment.yaml and test-environment.yaml). This creates divergent dependency sets between pip installs (setup.py) and conda-based CI/docs builds; consider either relaxing the conda pins too or keeping consistent bounds across packaging files.
install_requires = [
'pyyaml>=3.12',
'pint>=0.7.2',
'numpy>=1.13.0',
'tables',
'pyked>=0.4.1',
'scipy>=1.0.0',
'cantera>=2.3.0'
]
Correct comment spelling Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Merge comment spelling error resolved on GitHub
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
setup.py:36
install_requiresnow allows unbounded major upgrades forpyyaml,pint, andnumpy, but the conda/test environments in this repo still pin older upper bounds (e.g.,numpy<2.0,pint<0.9,pyyaml<4.0). As a result, CI won’t exercise the newer versions that pip users can now receive. Either keep the upper bounds here, or update the conda/test environment constraints (and CI matrix) to match the versions you intend to support.
install_requires = [
'pyyaml>=3.12',
'pint>=0.7.2',
'numpy>=1.13.0',
'tables',
'pyked>=0.4.1',
'scipy>=1.0.0',
'cantera>=2.3.0'
]
Comment on lines
+99
to
+100
| no_ignition_delay = np.array([0.0]) | ||
|
|
Comment on lines
+334
to
+346
| cwd = os.getcwd() | ||
| with TemporaryDirectory() as temp_dir: | ||
| os.chdir(temp_dir) | ||
| try: | ||
| ignition_delays = simulation.get_ignition_delay( | ||
| times, target, 'species', ignition_type | ||
| ) | ||
| dumped_files = [ | ||
| filename for filename in os.listdir(temp_dir) | ||
| if filename.startswith('target-data-') and filename.endswith('.out') | ||
| ] | ||
| finally: | ||
| os.chdir(cwd) |
Comment on lines
+17
to
23
| - python >=3.7,<3.10 | ||
| - setuptools | ||
| - pip | ||
|
|
||
| run: | ||
| - python | ||
| - python >=3.7,<3.10 | ||
| - pyyaml >=3.12,<4.0 |
Member
|
@LekiaAnonim let me know when this is ready for my final review - however, doesn't need to fix everything (e.g., the whole build system/CI needs to be updated) |
Contributor
Author
Alright. That's true. The major fixes have been made. |
Member
|
OK, I think this looks good |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
get_ignition_delay crashed with ValueError: attempt to get argmax of an empty sequence when detect_peaks returned no peaks (e.g. flat/non-igniting signal). Added peak_inds.size == 0 guards in all four ignition type branches so execution falls through to the existing sentinel return. Also relaxes upper version bounds on pyyaml/pint/numpy in setup.py and adds python_requires>=3.7.