Skip to content

v2: Clarify simulation file #632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 5, 2025
Merged

v2: Clarify simulation file #632

merged 5 commits into from
Jun 5, 2025

Conversation

dweindl
Copy link
Member

@dweindl dweindl commented May 21, 2025

  • Remove the simulation from the list of PEtab files, because it is not part of the parameter estimation problem.
  • Add a section under measurement table stating the expected column name.

Closes #628.

* Remove the simulation from the list of PEtab files, because it is not part of the parameter estimation problem.
* Add a section under *measurement table* stating the expected column name.

Closes #628.
@dweindl dweindl requested a review from a team as a code owner May 21, 2025 12:46
@dweindl dweindl added this to the PEtab 2.0.0 milestone May 21, 2025
@dweindl
Copy link
Member Author

dweindl commented May 21, 2025

To be more useful, I think also the actual noise value should be part of that table, at least for those cases where noiseFormula depends on the model state. But currently, I think, everything assumes that noiseParameters is a numeric value, or at least only depends on (constant) parameters.

@sebapersson
Copy link
Contributor

To be more useful, I think also the actual noise value should be part of that table, at least for those cases where noiseFormula depends on the model state. But currently, I think, everything assumes that noiseParameters is a numeric value, or at least only depends on (constant) parameters.

This is a good point that especially can help with testing. If it is included I think it is should be included for all measurement points.

@dweindl
Copy link
Member Author

dweindl commented May 22, 2025

Including the evaluated noise scale would require an additional column (simulationNoise?). Just writing that value to noiseParameters would be incorrect in all cases in which 'noiseFormulais not just a single parameter, and not having the originalnoiseFormula` would make it impossible to match the rows from the measurement table sometimes.

@sebapersson
Copy link
Contributor

sebapersson commented May 22, 2025

Including the evaluated noise scale would require an additional column (simulationNoise?).

Yes, that would require an extra column, which I do not know if we want to have if the table should match the measurements table as closely as possible. Maybe easiest to not include it in the current iteration?

@dweindl
Copy link
Member Author

dweindl commented May 22, 2025

Maybe easiest to not include it in the current iteration?

I'd rather remove any mention of the simulation file then until we converged on what it should look like. I am not sure it's helpful if different tools have different expectations of what columns are present...

@sebapersson
Copy link
Contributor

I do not even know if most tools support outputting the noise, while due to the test-suite most tools should be able to output simulated values. So feels it should be fine to keep simulation as it is, and in future iterations add noise-column potentially?

@dweindl
Copy link
Member Author

dweindl commented Jun 5, 2025

I do not even know if most tools support outputting the noise, while due to the test-suite most tools should be able to output simulated values. So feels it should be fine to keep simulation as it is, and in future iterations add noise-column potentially?

Okay, I'll add it as is for now.

@dweindl dweindl merged commit 23e1607 into main Jun 5, 2025
2 checks passed
@dweindl dweindl deleted the fix-628-simulation branch June 5, 2025 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

specify column name for simulation data for optional simulation.tsv
3 participants