Feature/generalized auralization by mberz · Pull Request #83 · choras-org/backend

mberz · 2026-05-04T10:38:47Z

Changes

Add generalized *.wav export after a simulation is finished in the SimulationService
Add a simplified and generalized mono-aural auralization function based on convolution with the RIR

These new functions should replace individual/method specific implementations which are based on simple convolution with the RIR returned by the simulation methods.

Superseeded by #90 and #91

…ods returning a RIR

…tion method and result export in the SimulationService and AuralizationService)

…side a docker container. Now the local version also works (for debugging the CHORAS backend).

…eaning up, as well as preventing errors with existing containers.

…oach. Also refactored a bit so that a not-working xlsx export will not break the auralization.

SilvinWillemsen · 2026-05-04T16:03:00Z

Hi @mberz,

I managed to make DG work with the Pyfar approach (commit 2fdaebd)

However, to make things work locally I had to make some changes in the LocalExecutor (commit 9cfc19c)

Also, I found a mistake when overriding a simulation: the container doesn’t get renamed and the simulation will not run. Removing the container after use solves this issue (commit 5f87fd7)

Hope it's ok to merge everything in this single PR!

mberz · 2026-05-05T09:35:59Z

Thanks for implementing @SilvinWillemsen

However, to make things work locally I had to make some changes in the LocalExecutor (commit 9cfc19c)

I'm not sure I'm getting the reason for this. Could you please add some details why this is required?

Also, I found a mistake when overriding a simulation: the container doesn’t get renamed and the simulation will not run. Removing the container after use solves this issue (commit 5f87fd7)

That's probably a bug introduced when cleaning up. Thanks for catching it.

SilvinWillemsen · 2026-05-05T09:42:32Z

I'm not sure I'm getting the reason for this. Could you please add some details why this is required?

Hi @mberz, with local debugging I mean breakpoint debugging in VScode. The old code only worked when running the CHORAS backend as a docker container. Does this make sense?

mberz · 2026-05-05T11:02:58Z

I'm not sure I'm getting the reason for this. Could you please add some details why this is required?

Hi @mberz, with local debugging I mean breakpoint debugging in VScode. The old code only worked when running the CHORAS backend as a docker container. Does this make sense?

Ah, got it. Could you in this case please add more documentation to this (clearly stating that this and which part of it is not production code)?

Copilot

Pull request overview

This PR aims to generalize post-simulation impulse-response .wav export and add a shared mono auralization path, while also improving local Docker path resolution and expanding executor tests. In the current form, the new execution/export flow introduces several blocking regressions around cloud runs, cancellation handling, and DG compatibility.

Changes:

Add automatic IR .wav export in run_solver() and new generic mono auralization support.
Refactor local executor mount resolution to better map container paths back to host paths.
Expand integration tests for local executor path-resolution behavior.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`app/services/simulation_service.py`	Adds new container cleanup and generalized IR `.wav` export logic after solver completion.
`app/services/auralization_service.py`	Adds fallback mono convolution-based auralization using exported `.wav` IR files.
`app/services/executors/local_executor.py`	Adds container-environment detection and improved host-path resolution for mounted volumes.
`tests/integration/test_local_executor_final.py`	Uncomments/adds path-resolution coverage for local executor behavior.
`requirements.txt`	Adds `pyfar` dependency for audio I/O and DSP.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+            match simulation_method:
+                case "DE":
+                    # TODO: This function is not a general auralization function and should be renamed
+                    imp_tot, fs = auralization_calculation(
+                        None,
+                        json_path.replace(".json", "_pressure.csv"),
+                        json_path.replace(".json", ".wav"),
+                    )
+
+                # this should be the only thing getting executed
+                case _:
+                    import numpy as np


+    def test_resolves_subdirectory_of_mount(self, mock_docker_client, container_with_mounts):
+        """
        U18 — EP-D1
        Container path is a subdirectory of a mount →
        resolved by computing relative suffix and appending to host source.
-
+        """
        mock_docker_client.containers.get.return_value = container_with_mounts
        with patch("socket.gethostname", return_value="my-container-id"):
            result = get_host_path_for_container_path("/app/uploads/subdir")
-        assert result == "/host/uploads/subdir" """
+        assert result == "/host/uploads/subdir"


+    import pyfar as pf
+    dry_signal = pf.io.read_audio(signal_file_name)
+    rir = pf.io.read_audio(impulse_response_file_name_wav)
+    rir_resampled = pf.dsp.resample(rir, dry_signal.sampling_rate)
+    convolved_signal = pf.dsp.convolve(rir_resampled, dry_signal)
+    pf.io.write_audio(convolved_signal, wav_output_file_name)
+
+


mberz

@SilvinWillemsen Thanks for working on this. I cannot request changes but only comment, since this PR was opened under my own account.
Could you have a look at the comments and make changes where required?

mberz · 2026-05-06T06:53:54Z

-    """def test_resolves_subdirectory_of_mount(self, mock_docker_client, container_with_mounts):
-
+    def test_resolves_subdirectory_of_mount(self, mock_docker_client, container_with_mounts):
+        """
        U18 — EP-D1
        Container path is a subdirectory of a mount →
        resolved by computing relative suffix and appending to host source.
-
+        """
        mock_docker_client.containers.get.return_value = container_with_mounts
        with patch("socket.gethostname", return_value="my-container-id"):
            result = get_host_path_for_container_path("/app/uploads/subdir")
-        assert result == "/host/uploads/subdir" """


Why has this been commented out? If it's not applicable anymore I think it should be adapted or removed. Very much preferably adapted and not removed.

This used to be commented out, but copilot suggested to uncomment it. What do you think?

Ah, I see. Let's leave it the way it used to be and create an issue for it. Then we can check further after the workshop.

mberz · 2026-05-06T06:58:19Z

+            # auralization: generate impulse response wav file
+            # TODO: fix DG method such that this auralization works,
+            # the idea is to have one shared pipeline across all
+            # methods. 


Could you adapt the comment and remove everything you completed in this PR?

mberz · 2026-05-06T07:01:07Z

+                    if imp_tot is None or len(imp_tot) == 0:
+                        logger.warning("Impulse response data is empty or missing")
+                        imp_tot = np.zeros(44100)  # 1 second of silence at 44.1 kHz
+                        norm_rir = pf.Signal(imp_tot, fs)
+                    else:
+                        rir = pf.Signal(imp_tot, fs)
+                        norm_rir = pf.dsp.normalize(rir)


Suggested change

if imp_tot is None or len(imp_tot) == 0:

logger.warning("Impulse response data is empty or missing")

imp_tot = np.zeros(44100) # 1 second of silence at 44.1 kHz

norm_rir = pf.Signal(imp_tot, fs)

else:

rir = pf.Signal(imp_tot, fs)

norm_rir = pf.dsp.normalize(rir)

if imp_tot is None or len(imp_tot) == 0:

logger.warning("Impulse response data is empty or missing")

imp_tot = np.zeros(44100) # 1 second of silence at 44.1 kHz

rir = pf.Signal(imp_tot, fs)

norm_rir = pf.dsp.normalize(rir)

I have a conceptual comment here:
Why does the RIR get normalized by default?
This removes for example any distance information in the auralization and source strength differences. Not sure if that's something that makes a lot of sense as default.

Maybe we can implement something like a normalize checkbox for the auralization in the future instead?

Hmm.. we need the normalisation because DG, for instance, returns crazy-large pressure values. This is a good comment though. We probably want to have a pressure to signal function (does pyfar have that?).

Regarding the changes you suggest, wouldn't the pf.dsp.normalize function return NaN if imp_tot is None because rir consists of zeros?

Ah, I see. Let's keep it at always normalize then.

The normalize function by default normalizes to the maximum. So in case the data consists of only zeros it will contain nans afterwards, indeed.

I'll add some comments, but will leave the functionality as is, alright? :)

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

…ext step is to unlink it at a later point Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

…e previous commit

This reverts commit e497841.

…t normalise a signal filled with 0s.

mberz

Hey @SilvinWillemsen I think there's a much simpler way to implement the removal of the container.
With your implementation I'm not even sure if everything will work with the cloud computation executor. I think it's best to encapsulate as much functionality specific to the executors in the respective classes to have to handle edge cases and exceptions in the runner function.

mberz · 2026-05-07T11:25:45Z

+            container = None
+            try:
+                container = executor.execute(method_config, sim_config)
+                container.wait()
+                logger.info(f"{simulation_method} Simulation_service:...container has finished.")
+            except Exception as ex:
+                logger.error(f"Error during container execution: {ex}")
+                raise Exception(f"Error during container execution: {ex}")
+            finally:
+                remove_method = getattr(container, "remove", None) if container is not None else None
+                if callable(remove_method):
+                    try:
+                        remove_method()  # Clean up local containers after execution
+                    except Exception as cleanup_ex:
+                        # If cancelled, the container is already removed, so this exception will be thrown.
+                        logger.warning(f"Failed to remove execution container: {cleanup_ex}")


To me it seems that all if this can be removed by uncommenting this code line here

backend/app/services/executors/local_executor.py

Line 191 in c48f87c

# remove = True,

So I would propose to revert this part.

mberz · 2026-05-07T11:25:50Z

-            container = executor.execute(method_config, sim_config)
-            container.wait()
-            logger.info(f"{simulation_method} Simulation_service:...container has finished.")
+            logger.info(f"{simulation_method} Simulation_service:...container has been spun up.")


To me it seems that all if this can be removed by uncommenting this code line here

backend/app/services/executors/local_executor.py

Line 191 in c48f87c

# remove = True,

So I would propose to revert this part.

mberz · 2026-05-07T11:26:44Z

+            cancel_flag_path = Path(json_path).parent / f"{result_container['task_id']}.cancel"
+


I think this can be reverted when using the remove functionality of the executor class

backend/app/services/executors/local_executor.py

Line 191 in c48f87c

# remove = True,

So I would propose to revert this part.

mberz · 2026-05-07T11:27:19Z

+            if os.path.exists(cancel_flag_path):
+                # Clean up cancel flag after handling cancellation
+                try:
+                    cancel_flag_path.unlink()
+                    logger.info(f"Removed cancel flag file: {cancel_flag_path}")
+                except Exception as ex:
+                    logger.warning(f"Failed to remove cancel flag file {cancel_flag_path}: {ex}")
+


Not sure if this can be reverted as well when using the remove option in the executor

## Changes - Add generalized *.wav export after a simulation is finished in the SimulationService - Add a simplified and generalized mono-aural auralization function based on convolution with the RIR These new functions should replace individual/method specific implementations which are based on simple convolution with the RIR returned by the simulation methods. Replaces #83

mberz added 4 commits May 4, 2026 12:26

Re-add the DE auralization method

edc4091

Add temporary general wav file writing functionality for all new meth…

af92d8c

…ods returning a RIR

Add initial draft of the mono-aural auralization method

6c21402

Add pyfar as dependency (used in the newly added generalized auraliza…

4d1c19f

…tion method and result export in the SimulationService and AuralizationService)

mberz assigned SilvinWillemsen May 4, 2026

mberz added the enhancement New feature or request label May 4, 2026

mberz added this to CHORAS planning May 4, 2026

github-project-automation Bot moved this to Backlog in CHORAS planning May 4, 2026

mberz moved this from Backlog to Implementation in progress in CHORAS planning May 4, 2026

SilvinWillemsen added 4 commits May 4, 2026 17:22

Added functionality to check whether we're running locally or from in…

9cfc19c

…side a docker container. Now the local version also works (for debugging the CHORAS backend).

Making sure that containers are removed after using them. Both for cl…

5f87fd7

…eaning up, as well as preventing errors with existing containers.

WIP: general auralization. DG now gets auralized using the Pyfar appr…

2fdaebd

…oach. Also refactored a bit so that a not-working xlsx export will not break the auralization.

Copilot docstring fix

e497841

mberz requested a review from Copilot May 5, 2026 12:03

Copilot started reviewing on behalf of mberz May 5, 2026 12:04 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

mberz commented May 6, 2026

View reviewed changes

SilvinWillemsen and others added 8 commits May 6, 2026 09:30

Added more documentation to state what part is used for local debugging.

29ad43f

Fix for container removal on the cloud (copilot)

335aa73

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Removed unlinking of the cancel flag as we're still using it later. N…

658b893

…ext step is to unlink it at a later point Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Added cancel flag removal (at a better place) after removing it in th…

49347e7

…e previous commit

Added comment on calling remove_method when cancelling

2f7bdd6

Updated todo

64961c2

Revert "Copilot docstring fix"

bbc8e0a

This reverts commit e497841.

Added comments describing the normalisation procedure and why we don'…

c48f87c

…t normalise a signal filled with 0s.

mberz commented May 7, 2026

View reviewed changes

mberz mentioned this pull request May 10, 2026

FEAT: General auralization #90

Merged

mberz moved this from Implementation in progress to On hold in CHORAS planning May 10, 2026

		cancel_flag_path = Path(json_path).parent / f"{result_container['task_id']}.cancel"

Conversation

mberz commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

SilvinWillemsen commented May 4, 2026

Uh oh!

mberz commented May 5, 2026

Uh oh!

SilvinWillemsen commented May 5, 2026

Uh oh!

mberz commented May 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mberz 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!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mberz 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

Development

Uh oh!

3 participants

mberz commented May 4, 2026 •

edited

Loading