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

Checklist for new submissions #206

Merged
merged 9 commits into from
Apr 18, 2024
Merged

Checklist for new submissions #206

merged 9 commits into from
Apr 18, 2024

Conversation

dilpath
Copy link
Collaborator

@dilpath dilpath commented Apr 17, 2024

No description provided.

@dilpath dilpath requested review from dweindl and m-philipps April 17, 2024 12:43
@dweindl
Copy link
Member

dweindl commented Apr 17, 2024

@dilpath
Copy link
Collaborator Author

dilpath commented Apr 17, 2024

Hm, why a new file instead of updating/referencing https://github.com/Benchmarking-Initiative/Benchmark-Models-PEtab/blob/master/.github/PULL_REQUEST_TEMPLATE/new_petab_problem.md?

Good point, I'll try to consolidate them. I don't see this template when I open a new PR, hopefully moving it to pull_request_template.md fixes that

@dweindl
Copy link
Member

dweindl commented Apr 17, 2024

Good point, I'll try to consolidate them.

👍

I don't see this template when I open a new PR, hopefully moving it to pull_request_template.md fixes that

That's weird. I think it was working at some point.

pull_request_template.md Outdated Show resolved Hide resolved
pull_request_template.md Outdated Show resolved Hide resolved
pull_request_template.md Outdated Show resolved Hide resolved
pull_request_template.md Outdated Show resolved Hide resolved
pull_request_template.md Outdated Show resolved Hide resolved
pull_request_template.md Outdated Show resolved Hide resolved
pull_request_template.md Outdated Show resolved Hide resolved
@dilpath
Copy link
Collaborator Author

dilpath commented Apr 18, 2024

  • Differences between the implementation and the original publication are described

Should that be in the issue? It might be preferable to have it in the SBML annotations?

  • There is a GitHub issue for this problem

Do we really need an issue? It makes sense if one starts to convert some published problem to PEtab, to avoid other people working on the same thing. But if somebody wants to submit their just published files, I don't think we need an issue for that.

  • Source of nominal parameters (e.g.: taken from the original publication, or from your own fitting)

SBML annotation instead of gh issue?

At the moment, we don't have a nice place to put metadata -- this might change with the PEtab Result format, but we will probably still need some YAML that has other metadata.

For example, I don't think annotating the SBML with the reference DOI is a good idea, since this collection may contain PySB and other valid "PEtab v2" models soon. For the same reason, I also think things like model ID should rather exist in the PEtab YAML -- i.e. the model should not be relied upon by PEtab for metadata. So, some YAML with reference DOI etc. makes most sense to me -- or we integrate this information directly into the PEtab YAML. I guess we could design a simple extension for the PEtab YAML in PEtab v2, for users to specify metadata like reference DOI and arbitrary notes, what do you think?

@dweindl
Copy link
Member

dweindl commented Apr 18, 2024

Agreed that it makes sense to have the info on the dataset, provenience information on nominal parameters, ... in the PEtab files, because it doesn't directly affect the SBML model.

I know that at some point we had a discussion on whether PEtab yaml file should allow adding additional values (either arbitrary additional entries, or at least something like annotations: ). I can't find a github issue on that, and I don't remember the conclusion. But if we allow that, we could easily add that information there, according to a custom schema defined here. It wouldn't really have to be a PEtab extension.

That's a different discussion, though. So ignore those comments for the purpose of this PR.

@dilpath dilpath requested a review from dweindl April 18, 2024 10:45
@dilpath dilpath merged commit 375d793 into master Apr 18, 2024
3 checks passed
@dilpath dilpath deleted the contributing branch April 18, 2024 11:30
@dilpath dilpath restored the contributing branch April 18, 2024 11:31
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.

2 participants