Feature/speed_of_sound #100

JudithvE · 2025-11-05T13:18:41Z

Which issue(s) are closed by this pull request?

Closes #95

Changes proposed in this pull request:

Added function to calculate speed of sound based on temperature

SitChri

Thanky you for your kind contribution.
Please see the atteched suggestions for minor changes of your pull request.

We are looking forward to your changed pull request.

Greetings
ASSA 2025 Teamn

SitChri · 2025-11-05T13:59:47Z

pyrato/parametric.py

+        c=343.2*sqrt((temperature-t0)/(20-t0))\n
+        t0=-273.15°C
+    """
+    speed_of_sound = 343.2 * np.sqrt((temperature + 273.15)/(20+ 273.15))


maybe add reference temperature as a variable t_0? same for reference of speed of sound...

Not maybe, definitely :)

We have carefully discussed your comment with our team, however, we prefer doing it our way, respectfully.

SitChri · 2025-11-05T14:03:06Z

pyrato/parametric.py

+    Equations
+    ---------
+    .. math::
+        c=343.2*sqrt((temperature-t0)/(20-t0))\n


it would look nicer if latex syntax is used for the formula =)

You are completely right, we will change our code correspondingly asap!

TimSchellekensLAV

Thank you for your contribution.
Request some latex formatting and restructuring of docstring

TimSchellekensLAV · 2025-11-05T14:15:56Z

pyrato/parametric.py

+    ----------
+    ISO 9613-1 (Formula A.5)
+    
+    Equations


Incorporate equations in head of docstring

Accoring to the numpy convention, which are referd in the pyfar guidelines the Equations can be added in a Notes section instead of an dedicated Equation section.
https://numpydoc.readthedocs.io/en/stable/format.html#notes

TimSchellekensLAV · 2025-11-05T14:16:43Z

pyrato/parametric.py

+    Equations
+    ---------
+    .. math::
+        c=343.2*sqrt((temperature-t0)/(20-t0))\n


Suggested change

c=343.2*sqrt((temperature-t0)/(20-t0))\n

c=343.2\sqrt((temperature-t_0)/(20-t_0))\n

TimSchellekensLAV · 2025-11-05T14:17:33Z

pyrato/parametric.py

+        c=343.2*sqrt((temperature-t0)/(20-t0))\n
+        t0=-273.15°C
+    """
+    speed_of_sound = 343.2 * np.sqrt((temperature + 273.15)/(20+ 273.15))


Not maybe, definitely :)

mjasins

Thank You for Your contribution, please consider our suggestions.

mjasins · 2025-11-05T14:17:53Z

pyrato/parametric.py

+    ----------
+    temperature : double
+        Temperature in degrees Celsius.
+


Please add explicit formula and cite properly the source, according to pyfar convention

mjasins · 2025-11-05T14:22:19Z

pyrato/parametric.py

+    Equations
+    ---------
+    .. math::
+        c=343.2*sqrt((temperature-t0)/(20-t0))\n


Please add more descriptive variable names for t0 and use another variables for "hard-coded" 343.2 m/s and 20 Celsius temperature

mjasins · 2025-11-05T14:23:31Z

pyrato/parametric.py

+        c=343.2*sqrt((temperature-t0)/(20-t0))\n
+        t0=-273.15°C
+    """
+    speed_of_sound = 343.2 * np.sqrt((temperature + 273.15)/(20+ 273.15))


Also consider adding variables for hard-coded exact values to be more understandable.

mberz

Thanks. I've added a minor comment on how to cite the reference.
Feel free to include it.

pyrato/parametric.py

mberz · 2025-11-05T14:59:11Z

pyrato/parametric.py


+def calculate_speed_of_sound(temperature):
+    r"""Calculate the speed of sound in air depending on the temperature.
+


Suggested change

The implementation is based on [#]_ .

Co-authored-by: Marco Berzborn <mberz@users.noreply.github.com>

TimSchellekensLAV

Thanks for the additional changes. Minor improvement

TimSchellekensLAV · 2025-11-05T15:08:59Z

pyrato/parametric.py

+    Equations
+    ---------
+    .. math::
+        c = c_0 \sqrt{\frac{temperature - T_0}{20 - T_0}}


Suggested change

c = c_0 \sqrt{\frac{temperature - T_0}{20 - T_0}}

c = c_0 \sqrt{\frac{T - T_0}{20 - T_0}}

ahms5

Thank you for you contribution, it looks already very good, I just have a few suggestions.

ahms5 · 2025-11-05T15:09:36Z

pyrato/parametric.py

+    ----------
+    ISO 9613-1 (Formula A.5)
+    
+    Equations


Accoring to the numpy convention, which are referd in the pyfar guidelines the Equations can be added in a Notes section instead of an dedicated Equation section.
https://numpydoc.readthedocs.io/en/stable/format.html#notes

ahms5 · 2025-11-05T15:10:39Z

pyrato/parametric.py

+        T_0=-273.15
+    .. math::
+        c_0 = 343.2


It might make sence to add a unit here or explain the exact meaning in an additional text

…pyrato into Feature/speed_of_sound

TimSchellekensLAV · 2025-11-06T11:02:29Z

parametric.py does not pass ruff check

TimSchellekensLAV · 2025-11-06T11:10:27Z

test_documentation_build-3.13 failed

TimSchellekensLAV

Thank you for your contribution.
Small comment on test.

TimSchellekensLAV · 2025-11-06T11:14:36Z

tests/test_speed_of_sound.py

+@pytest.mark.parametrize('T',[0,20,1000])
+def test_speed_of_sound(T):
+    speed=ra.parametric.calculate_speed_of_sound(T)
+    assert isinstance(T, (int, float))


redundant? Implicit in specification of param

Can you specify?

TimSchellekensLAV · 2025-11-06T11:16:20Z

tests/test_speed_of_sound.py

+import pyrato as ra
+import pytest
+
+@pytest.mark.parametrize('T',[0,20,1000])


Testing 2 situations in this statement: those who work (20, 1000) and those who fail (0). Extract?

No the zero should also work.

JudithvE added 3 commits November 5, 2025 14:15

Feature/speed_of_sound

58d876e

extra latex math

ad95f02

\n

6bdb0a7

SitChri reviewed Nov 5, 2025

View reviewed changes

TimSchellekensLAV suggested changes Nov 5, 2025

View reviewed changes

documentation latex syntax

4f05d1f

mjasins suggested changes Nov 5, 2025

View reviewed changes

JudithvE added 2 commits November 5, 2025 15:29

latex try 2

429ac5c

testing other equation

12282bf

ahms5 added the ASSA 2025 Issues and PRs related to the Open Source Software school at ASSA 2025 label Nov 5, 2025

JudithvE added 5 commits November 5, 2025 15:41

go back to first stage

d84eae8

r before the documentation

59ded35

final version

f8cf348

final version 2

253fa03

final version 3

e714941

mberz reviewed Nov 5, 2025

View reviewed changes

JudithvE requested review from SitChri, TimSchellekensLAV and mjasins November 5, 2025 15:02

Update pyrato/parametric.py

3a292fc

Co-authored-by: Marco Berzborn <mberz@users.noreply.github.com>

JudithvE requested a review from mberz November 5, 2025 15:04

TimSchellekensLAV suggested changes Nov 5, 2025

View reviewed changes

ahms5 reviewed Nov 5, 2025

View reviewed changes

JudithvE added 5 commits November 5, 2025 16:11

final final version

1047b9a

Merge branch 'Feature/speed_of_sound' of https://github.com/JudithvE/…

4c74ca8

…pyrato into Feature/speed_of_sound

final final version 2

c7acec6

final final version 4

d3c5ef5

Input validation: speed_of_sound

1fd2345

JudithvE requested review from TimSchellekensLAV and ahms5 November 6, 2025 10:54

ruff check passed

5ba1081

extra enter

6b1d526

TimSchellekensLAV suggested changes Nov 6, 2025

View reviewed changes

footnote addition

1e89eca

	c=343.2*sqrt((temperature-t0)/(20-t0))\n
	c=343.2\sqrt((temperature-t_0)/(20-t_0))\n


		def calculate_speed_of_sound(temperature):
		r"""Calculate the speed of sound in air depending on the temperature.

	c = c_0 \sqrt{\frac{temperature - T_0}{20 - T_0}}
	c = c_0 \sqrt{\frac{T - T_0}{20 - T_0}}

Feature/speed_of_sound #100

Are you sure you want to change the base?

Feature/speed_of_sound #100

Uh oh!

Conversation

JudithvE commented Nov 5, 2025

Which issue(s) are closed by this pull request?

Changes proposed in this pull request:

Uh oh!

SitChri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimSchellekensLAV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjasins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mberz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimSchellekensLAV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahms5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimSchellekensLAV commented Nov 6, 2025

Uh oh!

TimSchellekensLAV commented Nov 6, 2025

Uh oh!

TimSchellekensLAV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone