SciML / ReactionNetworkImporters.jl

Julia Catalyst.jl importers for various reaction network file formats like BioNetGen and stoichiometry matrices
https://docs.sciml.ai/ReactionNetworkImporters/stable/
MIT License
26 stars 8 forks source link

Change u₀ to u0 #107

Closed TorkelE closed 8 months ago

TorkelE commented 11 months ago

Related to https://github.com/SciML/ReactionNetworkImporters.jl/issues/104

This is highly breaking though.

isaacsas commented 11 months ago

If we want to add this there needs to be an overload so the old notation works and returns u0 too. Then this isn’t breaking anything.

TorkelE commented 11 months ago

Is that possible (without simultaneously supporting both fields)? I am only familiar with overloads for functions, so would be unsure how to do this.

isaacsas commented 11 months ago

See getproperty and setproperty.

TorkelE commented 11 months ago

But both u₀ and u0 which are given to these would have the same type (Symbol). Or you mean create new version of these functions on ParsedReactionNetwork that first check if the input is u₀, and if so calls it again with input u0?

codecov[bot] commented 11 months ago

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (039f31e) 83.76% compared to head (9f42497) 83.00%.

Files Patch % Lines
src/ReactionNetworkImporters.jl 60.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #107 +/- ## ========================================== - Coverage 83.76% 83.00% -0.76% ========================================== Files 4 4 Lines 351 359 +8 ========================================== + Hits 294 298 +4 - Misses 57 61 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

TorkelE commented 9 months ago

Updated. These overloads added:

# Ensures that `prnbng.u₀` works.
function Base.getproperty(prnbng::ParsedReactionNetwork, name::Symbol)
    if name === :u₀
        return getfield(prnbng, :u0)
    else
        return getfield(prnbng, name)
    end
end

# Ensures that `prnbng.u₀ = ...` works.
function Base.setproperty!(prnbng::ParsedReactionNetwork, name::Symbol, x)
    if name === :u₀
        return setfield!(prnbng, :u0, x)
    else
        return setfield!(prnbng, name, x)
    end
end

Given that parsed reaction networks are immutable, I don;t think the other one is needed though?

I have also added a couple of tests in the tests file to check that (for some loaded network)

@test isequal(prnbng.u0, prnbng.u₀)
TorkelE commented 8 months ago

This one should be ready now.