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

Change u₀ to u0 #107

Merged
merged 6 commits into from
Dec 30, 2023
Merged

Change u₀ to u0 #107

merged 6 commits into from
Dec 30, 2023

Conversation

TorkelE
Copy link
Member

@TorkelE TorkelE commented Oct 7, 2023

Related to #104

This is highly breaking though.

@isaacsas
Copy link
Member

isaacsas commented Oct 7, 2023

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
Copy link
Member Author

TorkelE commented Oct 7, 2023

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
Copy link
Member

isaacsas commented Oct 7, 2023

See getproperty and setproperty.

@TorkelE
Copy link
Member Author

TorkelE commented Oct 7, 2023

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
Copy link

codecov bot commented Oct 7, 2023

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TorkelE
Copy link
Member Author

TorkelE commented Nov 18, 2023

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₀)

Copy link
Member

@isaacsas isaacsas left a comment

Choose a reason for hiding this comment

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

One change then I'm happy to merge. Thanks!

src/u0_index_change.jl Outdated Show resolved Hide resolved
@TorkelE TorkelE closed this Dec 30, 2023
@TorkelE TorkelE reopened this Dec 30, 2023
@TorkelE
Copy link
Member Author

TorkelE commented Dec 30, 2023

This one should be ready now.

@isaacsas isaacsas merged commit eebc558 into SciML:master Dec 30, 2023
9 of 14 checks passed
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