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

fix bug with normalization of transitory sensitivities with interfaces #226

Merged
merged 1 commit into from
Feb 17, 2024

Conversation

mjohnson541
Copy link
Collaborator

Should fix #223

@mjohnson541 mjohnson541 requested a review from hwpang July 26, 2023 20:45
@mjohnson541
Copy link
Collaborator Author

@ChrisBNEU can you test if this solves your problem?

@mjohnson541 mjohnson541 force-pushed the fix_transitorysens_interfaces branch from e5e0c9a to b61fe28 Compare October 31, 2023 16:49
@@ -163,6 +163,11 @@ function normalizefulltransitorysensitivities!(dSdt,ssys::SystemSimulation,t)
@views ns = y[sim.domain.indexes[1]:sim.domain.indexes[2]]
@views dSdt[sim.domain.indexes[1]:sim.domain.indexes[2],:] ./= ns
end
for (i,inter) in enumerate(ssys.interfaces)
if isa(inter,AbstractReactiveInternalInterface)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add blank after comma

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

Comment on lines 211 to 212
for (i,inter) in enumerate(ssys.interfaces)
if isa(inter,AbstractReactiveInternalInterface)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add blanks after the commas here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

Comment on lines 186 to 190
dSdt .*= ssys.p[ind]
for sim in ssys.sims
if sim.domain.parameterindexes[1]+length(sim.species) < ind && sim.domain.parameterindexes[2] > ind
dSdt .*= ssys.p[ind]
end
@views ns = y[sim.domain.indexes[1]:sim.domain.indexes[2]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should revert this because we should only apply this correction to rate. The old way of implement will only happen once if the ind is within the particular simulation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@mjohnson541 mjohnson541 force-pushed the fix_transitorysens_interfaces branch from b61fe28 to b3d12d8 Compare February 4, 2024 00:28
@mjohnson541 mjohnson541 force-pushed the fix_transitorysens_interfaces branch from b3d12d8 to be40355 Compare February 8, 2024 17:31
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

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

Comparison is base (be21539) 49.70% compared to head (be40355) 49.65%.

Files Patch % Lines
src/TransitorySensitivities.jl 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #226      +/-   ##
==========================================
- Coverage   49.70%   49.65%   -0.05%     
==========================================
  Files          31       31              
  Lines        8138     8146       +8     
==========================================
  Hits         4045     4045              
- Misses       4093     4101       +8     

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

@mjohnson541 mjohnson541 merged commit a313530 into main Feb 17, 2024
2 of 4 checks passed
@mjohnson541 mjohnson541 deleted the fix_transitorysens_interfaces branch February 17, 2024 18:33
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.

No sensitivity for interface reactions
2 participants