-
Notifications
You must be signed in to change notification settings - Fork 0
cleanup use of logarithms #19
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
base: main
Are you sure you want to change the base?
Conversation
…ases. a couple of derivatives wrt log fixed....need to finish this part
| REAL_TYPE const speciesConcentration_i = exp( logPrimarySpeciesConcentrations[i] ); | ||
| REAL_TYPE const speciesConcentration_i = logmath::exp( logPrimarySpeciesConcentrations[i] ); | ||
| aggregatePrimarySpeciesConcentrations[i] = speciesConcentration_i; | ||
| dAggregatePrimarySpeciesConcentrationsDerivatives_dLogPrimarySpeciesConcentrations( i, i ) = speciesConcentration_i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be?
dAggregatePrimarySpeciesConcentrationsDerivatives_dLogPrimarySpeciesConcentrations( i, i ) = logmath::dWrtLogConst() * speciesConcentration_i;
azibitsker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went went through the changes and based on my current understanding of the framework, I think there is only one type of correction that I see in the derivative of aggregate wrt to log(C^primary) that should have a ln(10) factor in front of C_i.
| aggregatePrimarySpeciesConcentrations[i] = speciesConcentration_i; | ||
| mobileAggregatePrimarySpeciesConcentrations[i] = speciesConcentration_i; | ||
| dAggregatePrimarySpeciesConcentrationsDerivatives_dLogPrimarySpeciesConcentrations( i, i ) = speciesConcentration_i; | ||
| dMobileAggregatePrimarySpeciesConcentrationsDerivatives_dLogPrimarySpeciesConcentrations( i, i ) = speciesConcentration_i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, should it be?
dAggregatePrimarySpeciesConcentrationsDerivatives_dLogPrimarySpeciesConcentrations( i, i ) = logmath::dWrtLogConst() * speciesConcentration_i;
dMobileAggregatePrimarySpeciesConcentrationsDerivatives_dLogPrimarySpeciesConcentrations( i, i ) = logmath::dWrtLogConst() * speciesConcentration_i;
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #19 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 25 26 +1
Lines 830 743 -87
=========================================
- Hits 830 743 -87 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few places missing logmath::dWrtLogConst< RealType >() but multiplied back somewhere else, so the unit tests don't complain. I've left my comments in some places and will check all other places.
| data.reactionRatesDerivatives( r, i ) = data.reactionRatesDerivatives( r, i ) * exp( -data.speciesConcentration[i] ); | ||
| } | ||
|
|
||
| data.reactionRatesDerivatives( r, i ) = data.reactionRatesDerivatives( r, i ) * logmath::exp( -data.speciesConcentration[i] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| data.reactionRatesDerivatives( r, i ) = data.reactionRatesDerivatives( r, i ) * logmath::exp( -data.speciesConcentration[i] ); | |
| data.reactionRatesDerivatives( r, i ) = data.reactionRatesDerivatives( r, i ) * logmath::exp( -data.speciesConcentration[i] ) / logmath::dWrtLogConst< double >(); |
| for( IntType j = 0; j < numPrimarySpecies; ++j ) | ||
| { | ||
| dReactionRates_dLogPrimarySpeciesConcentrations( i, j ) = reactionRatesDerivatives( i, j + numSecondarySpecies ); | ||
| dReactionRates_dLogPrimarySpeciesConcentrations( i, j ) = logmath::dWrtLogConst< REAL_TYPE >() * reactionRatesDerivatives( i, j + numSecondarySpecies ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to account for logmath::dWrtLogConst< REAL_TYPE >() when we calculate reactionRatesDerivatives in kineticReactions::computeReactionRates instead of here.
Uh oh!
There was an error while loading. Please reload this page.