Skip to content

Conversation

lrennels
Copy link
Collaborator

@lrennels lrennels commented Nov 4, 2020

This PR handles the issue shown by #779 in the underlying function _perturb_param! called within @defsim. Broadly the issue arises when there is a need to use broadcasting. This is problematic because (1) broadcast assignment of TimestepIndex to TimestepArray does not have the proper underlying methods and (2) it is not always clear whether there is a need for .= or = even when there is no time index.

The current solution control-flow can be described as follows. It is more complex/deep than I'd like, but it's the simplest I have for now. NOTE FLIP Y AND N ON HAS TIME INDEX FORK

image

@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #780 (205cd85) into master (1b8672a) will increase coverage by 0.05%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #780      +/-   ##
==========================================
+ Coverage   78.74%   78.79%   +0.05%     
==========================================
  Files          35       35              
  Lines        3044     3061      +17     
==========================================
+ Hits         2397     2412      +15     
- Misses        647      649       +2     
Flag Coverage Δ
unittests 78.79% <94.44%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/mcs/montecarlo.jl 85.36% <94.44%> (-0.14%) ⬇️
src/core/defcomp.jl 82.24% <0.00%> (-0.11%) ⬇️
src/mcs/defmcs.jl 61.81% <0.00%> (+0.90%) ⬆️

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 1b8672a...205cd85. Read the comment docs.

@lrennels
Copy link
Collaborator Author

@ckingdon95 I worked on this today and removed a lot of the control flow and the try/catch. It should work properly now, could you take a look at the changes and see if you think there is a better way to simplify or move common cases out of loops etc.? It would be good to get some fresh eyes on it :)

@lrennels lrennels requested a review from corakingdon November 20, 2020 05:29
@lrennels lrennels marked this pull request as ready for review November 20, 2020 05:29
@lrennels
Copy link
Collaborator Author

@ckingdon95 the doctests are being dumb right now so I removed them, but I added an Issue to add them back as soon as I can! If this looks good we can merge it and tag a new version soon. I'll keep a close eye on performance going forward as well.

arnavgautam
arnavgautam previously approved these changes Dec 21, 2020
The next step is to run DICE using the provided API for the package:

```jldoctest tutorial2; output = false, filter = r".*"s
```julai
Copy link
Collaborator

Choose a reason for hiding this comment

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

"julia"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh shoot yes thanks, let me make sure the tests all pass then I'll ping you again

@lrennels lrennels merged commit 24e61f5 into master Dec 21, 2020
@lrennels lrennels deleted the broadcast branch December 21, 2020 23:35
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