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

Add writeSBML #143

Merged
merged 45 commits into from
Jul 14, 2022
Merged

Add writeSBML #143

merged 45 commits into from
Jul 14, 2022

Conversation

giordano
Copy link
Collaborator

@giordano giordano commented Aug 7, 2021

This is a very preliminary draft, it only takes units into account

EDIT by @exaexa :
Looking at the "dependency order" of stuff in SBML, I guess the good implementation path is:

  • parameters
  • units
  • Compartment
  • GeneProducts (mostly done, but gene products need to be added to an FBC package, not directly to the model now done, but it requires [SBML] Add patch to add new functions to C API JuliaPackaging/Yggdrasil#5011)
  • GeneProductAssociations
  • Math (should be mostly complete, needs double checking)
  • InitialAssignments
  • FunctionDefinitions (depends on Math)
  • Rules (depend on Math)
  • Species (almost done, needs writing the formula done and fully tested)
  • Reactions
  • Objectives (requires Add Objective struct #211)
  • Events
  • model metadata (default unit kinds etc)
  • whole model

@giordano giordano marked this pull request as draft August 7, 2021 23:55
@codecov
Copy link

codecov bot commented Aug 7, 2021

Codecov Report

Merging #143 (552c7f5) into master (b89c973) will increase coverage by 1.92%.
The diff coverage is 95.58%.

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
+ Coverage   91.57%   93.50%   +1.92%     
==========================================
  Files           9       10       +1     
  Lines         451      754     +303     
==========================================
+ Hits          413      705     +292     
- Misses         38       49      +11     
Impacted Files Coverage Δ
src/SBML.jl 100.00% <ø> (ø)
src/utils.jl 86.95% <ø> (+0.28%) ⬆️
src/writesbml.jl 94.11% <94.11%> (ø)
src/math.jl 95.23% <100.00%> (+9.52%) ⬆️
src/readsbml.jl 98.26% <100.00%> (+0.50%) ⬆️
src/structs.jl 100.00% <100.00%> (ø)
src/unitful.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b89c973...552c7f5. Read the comment docs.

@giordano giordano force-pushed the mg/writesbml branch 3 times, most recently from 6e30b7f to 5526a7b Compare August 8, 2021 00:09
@exaexa exaexa linked an issue Aug 8, 2021 that may be closed by this pull request
@giordano giordano force-pushed the mg/writesbml branch 3 times, most recently from 865a735 to 831bad8 Compare August 8, 2021 22:22
@exaexa exaexa added the enhancement New feature or request label Aug 9, 2021
@giordano giordano force-pushed the mg/writesbml branch 2 times, most recently from 76de7bb to 5b2918f Compare November 19, 2021 18:43
@giordano giordano force-pushed the mg/writesbml branch 2 times, most recently from e179814 to 91d8515 Compare December 21, 2021 11:15
@exaexa exaexa added this to the v1.0 milestone Jan 3, 2022
@giordano giordano force-pushed the mg/writesbml branch 2 times, most recently from 34a915e to 01d7110 Compare January 10, 2022 11:18
@giordano giordano force-pushed the mg/writesbml branch 3 times, most recently from c92b07b to 69c1db8 Compare May 19, 2022 14:52
@giordano giordano force-pushed the mg/writesbml branch 3 times, most recently from 25619c1 to b9b63a0 Compare May 20, 2022 12:44
Project.toml Outdated Show resolved Hide resolved
src/writesbml.jl Outdated Show resolved Hide resolved
@giordano giordano force-pushed the mg/writesbml branch 2 times, most recently from 07b232e to 7ad6d13 Compare May 23, 2022 16:30
These methods aren't needed by the package at the moment and we can avoid
loading some extra packages (as small as they are).  If we need, we can move the
code back to the main package at any time.
This improves time-to-first `readSBML` a little bit, as the function is actually
run and fully compiled.  In particular, this reduces memory allocations a little
bit.

Without any precompilation:

```
julia> @time @eval using SBML
  0.358911 seconds (774.55 k allocations: 56.314 MiB, 1.73% compilation time)

julia> @time @eval m2 = readSBMLFromString(writeSBML(readSBML("Dasgupta2020.xml")))
  4.058562 seconds (3.89 M allocations: 253.997 MiB, 2.78% gc time, 98.77% compilation time)
SBML.Model with 6 reactions, 2 species, and 8 parameters.

julia> @time @eval m2 = readSBMLFromString(writeSBML(readSBML("Dasgupta2020.xml")))
  0.035276 seconds (3.74 k allocations: 174.250 KiB)
SBML.Model with 6 reactions, 2 species, and 8 parameters.
```

Before this change:

```
julia> @time @eval using SBML
  0.408944 seconds (864.25 k allocations: 62.282 MiB, 1.52% compilation time)

julia> @time @eval m2 = readSBMLFromString(writeSBML(readSBML("Dasgupta2020.xml")))
  3.576220 seconds (1.78 M allocations: 115.261 MiB, 1.53% gc time, 98.38% compilation time)
SBML.Model with 6 reactions, 2 species, and 8 parameters.

julia> @time @eval m2 = readSBMLFromString(writeSBML(readSBML("Dasgupta2020.xml")))
  0.033041 seconds (5.38 k allocations: 199.828 KiB)
SBML.Model with 6 reactions, 2 species, and 8 parameters.
```

After this change:

```
julia> @time @eval using SBML
  0.432498 seconds (877.53 k allocations: 63.441 MiB, 1.43% compilation time)

julia> @time @eval m2 = readSBMLFromString(writeSBML(readSBML("Dasgupta2020.xml")))
  3.197203 seconds (733.27 k allocations: 47.346 MiB, 0.72% gc time, 98.54% compilation time)
SBML.Model with 6 reactions, 2 species, and 8 parameters.

julia> @time @eval m2 = readSBMLFromString(writeSBML(readSBML("Dasgupta2020.xml")))
  0.034096 seconds (3.68 k allocations: 172.438 KiB)
SBML.Model with 6 reactions, 2 species, and 8 parameters.
```
@paulflang
Copy link
Collaborator

@giordano : would it be hard to merge the new Trigger struct and associated code before this whole PR is done? (Not super urgent, but would be nice to have)

@giordano
Copy link
Collaborator Author

giordano commented Jul 13, 2022

Rebasing on master would be highly non-fun, I had to do it a few times already and I'm not looking forward to doing it again.

@paulflang
Copy link
Collaborator

Alright. Then I'll wait and watch this space.

@exaexa
Copy link
Collaborator

exaexa commented Jul 14, 2022

@paulflang please open a PR with the code so that it stays in sight and we can see that we're not doing something that would break the code. Integrity of writeSBML merge is a kinda priority now (partly because how huge this PR is) -- I'll eventually do the gluing&rebasing with the Trigger code later.

@exaexa
Copy link
Collaborator

exaexa commented Jul 14, 2022

Depends on sbmlteam/libsbml#247

@exaexa
Copy link
Collaborator

exaexa commented Jul 14, 2022

Full functionality depends on solving sbmlteam/libsbml#248

@giordano giordano marked this pull request as ready for review July 14, 2022 14:04
Copy link
Collaborator

@exaexa exaexa left a comment

Choose a reason for hiding this comment

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

🎉 C E L E B R A T I O N 🎉

I'll eventually need to check whether some of the smaller items that are now created by plain constructor (Something_create) wouldn't be safer to get created by parent (inheriting XML props etc, ParentObject_addSomething style). But that's a completely minor problem.

Let's roll out 1.0 asap.

@exaexa exaexa merged commit 9f374c1 into LCSB-BioCore:master Jul 14, 2022
@giordano giordano deleted the mg/writesbml branch July 14, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ❌ breaking ❌ Needs a considerable version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Events: Trigger may need to be a struct writeSBML
3 participants