mimiframework / Mimi.jl

Integrated Assessment Modeling Framework
https://www.mimiframework.org
Other
66 stars 34 forks source link

WIP Use Different Approach to Check for Broadcasting in Perturb Param #999

Closed lrennels closed 2 months ago

lrennels commented 2 months ago

@jrising work in progress but I think this will solve https://forum.mimiframework.org/t/setting-a-distributions-over-large-dimensions-in-defsim/277/10

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.81%. Comparing base (6217072) to head (f719197). Report is 11 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #999 +/- ## ======================================= Coverage 84.81% 84.81% ======================================= Files 40 40 Lines 4062 4062 ======================================= Hits 3445 3445 Misses 617 617 ``` | [Flag](https://app.codecov.io/gh/mimiframework/Mimi.jl/pull/999/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mimiframework) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/mimiframework/Mimi.jl/pull/999/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mimiframework) | `84.81% <100.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mimiframework#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jrising commented 2 months ago

This seems like the right solution to me. Did you figure out if it's related to String vs. String15's? I thought it was due to my use of a single dimension when you just used this for double dimension parameters.

lrennels commented 2 months ago

I think jt is something to do with the String15, because I use this approach for single dimensions too, that’s how I did my comparison I ran this step by step alongside the same for the mortality Beta in GIVE and it seems String15 returns a vector for a single index when String returns an Int. I use CSVFiles and I have never seen that load a String type other than the base String.

But I think this solution gets closer to the heart of when Julia says you should broadcast so that’s better regardless? The best move would be for me to chase down in Julia base the check it does to throw the broadcast warning and copy that. I’ll do so. We have dreams of really simplifying our approach to these Arrays so they’re less custom … but for now this works

lrennels commented 2 months ago

We also used to use a recursive approach but I found that a bit convoluted.

lrennels commented 2 months ago

@davidanthoff let's take a look at this together, I'm not fully satisfied with the current implementation, feels clunky and like we should simplify the indexing here

lrennels commented 2 months ago

@jrising merging, will return to this to think about a more Julia-esque arrays way to handle it