feat: add numerical laplace transform#14602
feat: add numerical laplace transform#14602Tushar-R-Tyagi wants to merge 12 commits intoTheAlgorithms:masterfrom
Conversation
for more information, see https://pre-commit.ci
|
pre-commit.ci autofix |
There was a problem hiding this comment.
Pull request overview
Adds a numerical (sampled-data) Laplace transform implementation to the maths/ algorithms collection.
Changes:
- Introduces
laplace_transform()that applies an exponential kernel and integrates via the trapezoidal rule. - Adds doctest examples for two basic reference functions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if s_value < 0: | ||
| raise ValueError("s_value must be non-negative for convergence.") |
There was a problem hiding this comment.
The convergence check if s_value < 0: raise ... is not generally correct for the Laplace transform (e.g., for f(t)=e^{-t}, the transform converges for Re(s) > -1). Also, since this implementation integrates over a finite sampled window, negative s_value won't inherently diverge. Consider removing this restriction, or (if you keep it) documenting it as a deliberate limitation rather than a convergence requirement.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| >>> abs(res - 0.5) < 1e-3 | ||
| True | ||
| """ | ||
| if delta_t <= 0: |
There was a problem hiding this comment.
Good input validation — catching non-positive delta_t early prevents
silent numerical errors. Consider also validating that s_value is
non-negative since the docstring on line 18 states this implementation
only supports non-negative s values, but there is no guard for it:
if s_value < 0:
raise ValueError("s_value must be non-negative for this implementation.")
There was a problem hiding this comment.
Added validation for s_value < 0 with a ValueError. Thanks for this catch.
| raise ValueError("function_values array cannot be empty.") | ||
|
|
||
| # Time vector corresponding to the function values | ||
| time_vector = np.arange(len(function_values)) * delta_t |
There was a problem hiding this comment.
Using np.arange(len(function_values)) * delta_t works correctly but
np.linspace would be more semantically clear and consistent with the
doctests which already use np.linspace:
time_vector = np.linspace(0, (len(function_values) - 1) * delta_t,
len(function_values))
This makes the time vector construction more readable and explicit.
There was a problem hiding this comment.
I agree, Linspace is much clearer. I have replaced arange with linspace to make the time vector construction more readable.
| @@ -0,0 +1,66 @@ | |||
| """ | |||
| This module provides a numerical implementation of the Laplace Transform. | |||
There was a problem hiding this comment.
The module docstring is minimal — just one line and a Wikipedia link.
Consider expanding it to match the quality of the function docstring:
"""
Laplace Transform — Numerical Implementation.
Computes the numerical Laplace Transform using the trapezoidal
integration rule. Supports real-valued, non-negative Laplace
parameters only.
Reference: https://en.wikipedia.org/wiki/Laplace_transform
"""
There was a problem hiding this comment.
Thanks for this suggestion. I have updated the module just like you recommended.
|
Hi @Tushar-R-Tyagi, nice fix! I noticed the issue (#14606) mentions enforcing validation of non-negative
Happy to extend any help if needed. |
Updated module docstring, added validation for non-negative s_value, and replaced arrange with linspace for clarity.
for more information, see https://pre-commit.ci
Removed extra blank lines and cleaned up docstring.
for more information, see https://pre-commit.ci
|
ruff check . |
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
| @@ -0,0 +1,74 @@ | |||
| """ | |||
There was a problem hiding this comment.
An error occurred while parsing the file: maths/laplace_transformation.py
Traceback (most recent call last):
File "/opt/render/project/src/algorithms_keeper/parser/python_parser.py", line 146, in parse
reports = lint_file(
^^^^^^^^^^
libcst._exceptions.ParserSyntaxError: Syntax Error @ 1:1.
tokenizer error: no matching outer block for dedent
"""
^
Describe your change:
I have added a numerical implementation of the Laplace transform in the maths/ directory.
Checklist: