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

Indexing fixes #408

Merged
merged 9 commits into from
Mar 25, 2024
Merged

Indexing fixes #408

merged 9 commits into from
Mar 25, 2024

Conversation

isaacsas
Copy link
Member

No description provided.

@isaacsas
Copy link
Member Author

@AayushSabharwal can you check I've got the correct dispatches now for mutating parameters within JumpProblems ensuring update_parameters!(...) is called to update the associated MassActionJump?

@isaacsas
Copy link
Member Author

If this looks ok I'll add some more symbolic tests for integrators and for remake.

@coveralls
Copy link

coveralls commented Mar 20, 2024

Pull Request Test Coverage Report for Build 8363146682

Details

  • 5 of 7 (71.43%) changed or added relevant lines in 1 file are covered.
  • 103 unchanged lines in 27 files lost coverage.
  • Overall coverage decreased (-2.6%) to 86.02%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/problem.jl 5 7 71.43%
Files with Coverage Reduction New Missed Lines %
src/spatial/utils.jl 1 91.67%
src/spatial/topology.jl 1 93.44%
src/spatial/bracketing.jl 2 72.0%
src/spatial/reaction_rates.jl 2 94.29%
src/extended_jump_array.jl 2 78.21%
src/spatial/hop_rates.jl 2 98.88%
src/problem.jl 2 66.84%
src/spatial/spatial_massaction_jump.jl 3 85.45%
src/aggregators/frm.jl 3 95.38%
src/aggregators/bracketing.jl 3 90.0%
Totals Coverage Status
Change from base Build 8038301646: -2.6%
Covered Lines: 2209
Relevant Lines: 2568

💛 - Coveralls

Comment on lines +128 to +135
function SII.set_parameter!(prob::JumpProblem, val, idx)
ans = SII.set_parameter!(SII.parameter_values(prob), val, idx)

if using_params(prob.massaction_jump)
update_parameters!(prob.massaction_jump, prob.prob.p)
end

ans
Copy link
Member

Choose a reason for hiding this comment

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

While this is technically correct, set_parameter! will be called once for each parameter which would make this very expensive. I'll PR to SII to add a callback that runs at the end of setp (finalize_parameters_hook!(prob, ps)?)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I guess we could also use that for calling reset_aggregated_jumps! when integrators get updated in callbacks, which means one less thing for users to have to handle.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AayushSabharwal please ping me when a SII release with that callback is available so I can get this finished -- we need this for making updates to Catalyst for MTK9. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

It's been released now @isaacsas

Copy link
Member

@AayushSabharwal AayushSabharwal Mar 25, 2024

Choose a reason for hiding this comment

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

Oops, I thought it was merged and would be available. Sorry 😅

Copy link
Member

Choose a reason for hiding this comment

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

Now it's out in 0.3.13

@ChrisRackauckas ChrisRackauckas merged commit 53772a0 into SciML:master Mar 25, 2024
5 of 6 checks passed
@isaacsas
Copy link
Member Author

@ChrisRackauckas this hadn't yet been updated for SciML/SymbolicIndexingInterface.jl#58, which is the interface we are supposed to use (and was only merged/released today).

I'll open another PR to update this to the correct interface, hopefully later today or tomorrow.

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.

4 participants