Skip to content
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

PEtab YAML formatting #4

Closed
6 tasks
FFroehlich opened this issue Dec 2, 2024 · 5 comments · Fixed by #10
Closed
6 tasks

PEtab YAML formatting #4

FFroehlich opened this issue Dec 2, 2024 · 5 comments · Fixed by #10

Comments

@FFroehlich
Copy link
Contributor

FFroehlich commented Dec 2, 2024

I identified a couple of issues with the petab files for the test cases

  • all observable ids have the same name as the model entity, which is afaik not valid petab

  • we should use format_version 2.0.0 as mapping tables are not available with 1.0.0

  • parameter values for neural networks are provided in a separate parameters_nn.tsv file, how about directly creating a proper parameters table where test values are set as nominal values?

Code to implement a majority of the proposed changes is available here https://github.com/AMICI-dev/AMICI/blob/b201f4efc354939ac1b0763c65f2e22507d086c4/tests/sciml/testsuite.py#L177

@sebapersson
Copy link
Owner

I am also not sure whether we should make assignments to petab variables in the mapping table (model outputs to petab variables), this should happen in the condition table (which would also be beneficial to specify things like time derivatives etc,

If I understand this point correctly, you are only here referring to network outputs?

Anyhow, I think we should still allow this for two reasons. First, in the case of a single experimental condition, it is beneficial for users to avoid dealing with an additional table. Second, if the output is consistent across all conditions, it feels cumbersome to repeatedly define it in the conditions table.

For greater flexibility once https://github.com/PEtab-dev/PEtab/issues/586)](https://github.com/PEtab-dev/PEtab/issues/586 is resolved, we should also support having the output in the conditions table (but there are potential ambiguities in the PEtab spec here with for example rate, so I want to wait for this to be finalized before adding anything here). I can add test cases for this once the details are finalized in PEtab v2. Therefore, I suggest creating a separate issue for this, and then add test-cases once the v2 spec is done. What do you think?

@FFroehlich
Copy link
Contributor Author

I am also not sure whether we should make assignments to petab variables in the mapping table (model outputs to petab variables), this should happen in the condition table (which would also be beneficial to specify things like time derivatives etc,

If I understand this point correctly, you are only here referring to network outputs?

Anyhow, I think we should still allow this for two reasons. First, in the case of a single experimental condition, it is beneficial for users to avoid dealing with an additional table. Second, if the output is consistent across all conditions, it feels cumbersome to repeatedly define it in the conditions table.

But you ultimately run into the same issues that are being addressed in PEtab v2. are those assignments initial conditions, constant values, time derivatives etc. I would argue that having a logical and easy to understand syntax is probably better for users than saving some tables for some use cases.

For greater flexibility once [https://github.com/PEtab-dev/PEtab/issues/586)](https://github.com/PEtab-dev/PEtab/issues/586](https://github.com/PEtab-dev/PEtab/issues/586)%5D(https://github.com/PEtab-dev/PEtab/issues/586) is resolved, we should also support having the output in the conditions table (but there are potential ambiguities in the PEtab spec here with for example rate, so I want to wait for this to be finalized before adding anything here). I can add test cases for this once the details are finalized in PEtab v2. Therefore, I suggest creating a separate issue for this, and then add test-cases once the v2 spec is done. What do you think?

Yes waiting until the respective spec is finalized sounds good, but I thought this was already pretty close to being agreed to.

@sebapersson
Copy link
Owner

But you ultimately run into the same issues that are being addressed in PEtab v2. are those assignments initial conditions, constant values, time derivatives etc. I would argue that having a logical and easy to understand syntax is probably better for users than saving some tables for some use cases.

Fair point, and in hindsight having these things explicitly stated will help in the parsing. Then, lets go with this.

So how about this. We wait for PEtab-dev/PEtab#586 to be finalized (to avoid surprises). Meanwhile I will keep the mapping table like now, address the other points in this issue, and open a new issue on adapting to the new condition table format.

@FFroehlich
Copy link
Contributor Author

But you ultimately run into the same issues that are being addressed in PEtab v2. are those assignments initial conditions, constant values, time derivatives etc. I would argue that having a logical and easy to understand syntax is probably better for users than saving some tables for some use cases.

Fair point, and in hindsight having these things explicitly stated will help in the parsing. Then, lets go with this.

So how about this. We wait for PEtab-dev/PEtab#586 to be finalized (to avoid surprises). Meanwhile I will keep the mapping table like now, address the other points in this issue, and open a new issue on adapting to the new condition table format.

sounds good!

@dilpath
Copy link
Collaborator

dilpath commented Dec 17, 2024

Yes waiting until the respective spec is finalized sounds good, but I thought this was already pretty close to being agreed to.

At this point, unless someone complains, it will have the proposed structure, with some naming changes to make things more intuitive. Daniel worked on this a bit more in the last couple of weeks.

So how about this. We wait for PEtab-dev/PEtab#586 to be finalized (to avoid surprises). Meanwhile I will keep the mapping table like now, address the other points in this issue, and open a new issue on adapting to the new condition table format.

👍

This was referenced Dec 18, 2024
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 a pull request may close this issue.

3 participants