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

Changes for VectorContinuousCallback #754

Merged
merged 14 commits into from Jun 6, 2019
Merged

Conversation

kanav99
Copy link
Contributor

@kanav99 kanav99 commented May 30, 2019

previous_condition::Array{uType}
callback_cache::CallbackCacheType
previous_condition::CallbackCacheType
callback_next_sign::CallbackCacheType
Copy link
Member

Choose a reason for hiding this comment

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

This can be a bitarray?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this can be. This was defaulted to float previously thats why I chose float. Lets change this to BitArray.

src/solve.jl Outdated
@@ -156,6 +156,13 @@ function DiffEqBase.__init(

callbacks_internal = CallbackSet(callback,prob.callback)

max_len = DiffEqBase.max_vector_callback_length(callbacks_internal)
if max_len !== -1
Copy link
Member

Choose a reason for hiding this comment

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

not a huge deal, but breaks inference. Can we instead just search for the existence of a VectorContinuousCallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how does it break inference, return type is always Int64? I would need the max length out of all VectorContinuousCallback to make the CallbackCache.

Copy link
Member

Choose a reason for hiding this comment

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

max_len cannot be inferred since it's a value, and then which of the branches is taken isn't inferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how do you propose to do this? I think Val{} might not be of use as well

Copy link
Member

Choose a reason for hiding this comment

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

Look for VectorContinuousCallback. That is type information.

@kanav99 kanav99 closed this Jun 5, 2019
@kanav99 kanav99 reopened this Jun 5, 2019
@ChrisRackauckas
Copy link
Member

Put a lower bound on DiffEqBase v5.9.0

@kanav99
Copy link
Contributor Author

kanav99 commented Jun 5, 2019

I have made another PR (SciML/DiffEqBase.jl#231), please tag after that is approved and merge.

@coveralls
Copy link

Coverage Status

Coverage decreased (-20.9%) to 49.093% when pulling b2fd928 on vector_callbacks into eaf3b23 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-20.9%) to 49.093% when pulling b2fd928 on vector_callbacks into eaf3b23 on master.

@coveralls
Copy link

coveralls commented Jun 5, 2019

Coverage Status

Coverage increased (+14.7%) to 68.26% when pulling 9e85dfc on vector_callbacks into d5b9534 on master.

@kanav99
Copy link
Contributor Author

kanav99 commented Jun 6, 2019

Oh god this is bad. Making fixes.

@codecov
Copy link

codecov bot commented Jun 6, 2019

Codecov Report

Merging #754 into master will decrease coverage by 1.53%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #754      +/-   ##
==========================================
- Coverage   72.37%   70.84%   -1.54%     
==========================================
  Files          89       89              
  Lines       29039    29328     +289     
==========================================
- Hits        21017    20777     -240     
- Misses       8022     8551     +529
Impacted Files Coverage Δ
src/integrators/type.jl 100% <ø> (ø) ⬆️
src/integrators/integrator_utils.jl 78.73% <ø> (-10.33%) ⬇️
src/solve.jl 90% <100%> (-1.38%) ⬇️
src/composite_algs.jl 0% <0%> (-96%) ⬇️
src/dense/high_order_rk_addsteps.jl 5.19% <0%> (-92.21%) ⬇️
src/perform_step/composite_perform_step.jl 0% <0%> (-78.27%) ⬇️
src/dense/low_order_rk_addsteps.jl 15.33% <0%> (-74.57%) ⬇️
src/integrators/integrator_interface.jl 4.95% <0%> (-70.25%) ⬇️
src/dense/stiff_addsteps.jl 4.44% <0%> (-67.23%) ⬇️
src/dense/verner_addsteps.jl 38.62% <0%> (-61.06%) ⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaf3b23...b2fd928. Read the comment docs.

@ChrisRackauckas ChrisRackauckas merged commit 8e110ca into master Jun 6, 2019
@ChrisRackauckas ChrisRackauckas deleted the vector_callbacks branch June 6, 2019 12:45
@ChrisRackauckas
Copy link
Member

That last test is fixed by SciML/DiffEqBase.jl#238

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.

None yet

4 participants