Skip to content

Conversation

SimonBuechner
Copy link

fixture added in conftest.py, test_parameters.py adjusted to contain fixture-tests for clarity

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

Closes #81

Changes proposed in this pull request:

  • make_edc_from_energy fixture implemented in conftest.py
  • test_parameters.py added and fixture-adjusted clarity-tests added.
  • newly proposed testing structure reducing redundancy

@SimonBuechner SimonBuechner added this to the v1.0.0 milestone Oct 13, 2025
@SimonBuechner SimonBuechner self-assigned this Oct 13, 2025
@SimonBuechner SimonBuechner added enhancement New feature or request question Further information is requested v1.0.0 labels Oct 13, 2025
@SimonBuechner SimonBuechner marked this pull request as draft October 13, 2025 12:48
@SimonBuechner
Copy link
Author

As requested this is the proposed fixture for the parameter tests. A few conceptual problems though:

  • it basically just builds a pyfar.TimeData object from provided energy and samplerate. It also normalizes the energy although, as we talked about this before, this is not neccessary.
  • In that case though, this could be done inline by just calling the TimeData-Constructor – especially because we need the energy values in many cases anyways to calculate the solution.
  • We should think about keeping it anyways for future cases.
  • The whole thing will be restructured anyways because as soon as the _energy_balance (V1 parameter definition #80) base function is done we will need slightly different tests

Before continuing though I'd love some feedback on this.

Copy link
Member

@ahms5 ahms5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thanks, I have some conseptual questions.
would it make sence to define different fixtures with more describing names for energy, "geometric" and "real"?
could you also add more details to what geometic and real means?

@github-project-automation github-project-automation bot moved this from Backlog to Require review in Weekly Planning Oct 14, 2025
@SimonBuechner
Copy link
Author

nice, thanks, I have some conseptual questions. would it make sence to define different fixtures with more describing names for energy, "geometric" and "real"? could you also add more details to what geometic and real means?

Yes, will do. I think I will also allow handover of e.g. the parameters for the geometrical decay because we usually need them in the tests anyway.

Right now it's a rough sketch to get the idea across. I was thinking about implementing different fixtures for those cases but I like the idea of having one universal fixture to get different EDCs for these kind of tests.

@SimonBuechner SimonBuechner requested a review from ahms5 October 14, 2025 15:24
Copy link
Member

@mberz mberz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this started.

In general I feel that the fixture is more complex than it needs to be and can be simplified, see the comments below.
I would propose to reduce the number of different parameterizations to for example:

  • passing energy values which represent an edc
  • passing a reverberation time/ intercept (simple exponential decay function)

In most test cases, I'd expect that an edc with a dynamic range between 0 and -65 (or a bit more headroom) is sufficient. I don't think there's need for -inf dB dynamic range for the average test case. This in my opinion could be a more representative default.

Currently, the fixture is not tested, please add some basic tests for the fixture.
Please also do not mix up adding this fixture with other changes such as changing the tests for other functions at this point.

@SimonBuechner
Copy link
Author

I restructured the factory so that we follow a more generalized approach for an exponential decay as requested. For test convenience it now returns a simple empty edc for typechecks etc.

dynamic range can also be adjusted - not sure though if this is neccessary but it leaves flexibility.

More tests for the fixture will follow.

@SimonBuechner SimonBuechner requested a review from mberz October 22, 2025 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request question Further information is requested v1.0.0

Projects

Status: Require review

Development

Successfully merging this pull request may close these issues.

3 participants