Skip to content

Conversation

lrennels
Copy link
Collaborator

This handles #754, originally pointed out on the forum at https://forum.mimiframework.org/t/set-a-parameter-as-a-distribution/106/15.

We need to handle the issue of parameterizing a Parameter or Distribution with a type from another package like Distributions, specifically something like this:

lws_distribution = Parameter{Distributions.Normal{Float64}}()

The error originally stems from like 271 of @defcomp:

datum_type = (datum_type === nothing ? Number : Main.eval(datum_type))

@davidanthoff suggests the following : "We can discuss the problem with Distributions on Fri. We might just have slightly rewrite the expression that gets passed to the Main.eval call, so that in the expression tree we don't use a symbol Distributions (which it will then try to resolve in the Main module, where Distributions isn't loaded), but instead just put a ref to the Distributions module itself into the AST that we pass to eval. I think it should then work without a name resolution."

@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #757 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #757      +/-   ##
==========================================
+ Coverage   77.90%   77.91%   +0.01%     
==========================================
  Files          36       36              
  Lines        2996     2998       +2     
==========================================
+ Hits         2334     2336       +2     
  Misses        662      662              
Flag Coverage Δ
#unittests 77.91% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
src/core/defcomp.jl 83.90% <100.00%> (+0.18%) ⬆️

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 dd347b5...435763f. Read the comment docs.

@lrennels
Copy link
Collaborator Author

lrennels commented Sep 10, 2020

@davidanthoff the calls look like this:

@debug "before: datum type is $datum_type, which is of type $(typeof(datum_type))"
datum_type = (datum_type === nothing ? Number : Main.eval(datum_type))
@debug "after: datum type is $datum_type, which is of type $(typeof(datum_type))"

which returns

┌ Debug: before: datum type is Distributions.Normal{Float64}, which is of type Expr
└ @ Mimi ~/.julia/dev/Mimi/src/core/defcomp.jl:277
┌ Debug: after: datum type is Normal{Float64}, which is of type DataType
└ @ Mimi ~/.julia/dev/Mimi/src/core/defcomp.jl:279

I think that makes sense, since if you call Main.eval(Distributions.Normal{Float64}) without Distributions loaded you get an error. This means we're calling Main.eval within @defcomp, but I'm not quite sure what to do from here ... something with modules perhaps.

I also added a wip file with I think is what we want to work?

@lrennels lrennels marked this pull request as ready for review September 10, 2020 20:23
@lrennels
Copy link
Collaborator Author

@davidanthoff ok I think this works ...

Copy link
Collaborator

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@lrennels lrennels merged commit 04a50cb into master Sep 11, 2020
@davidanthoff davidanthoff deleted the eval-distributions branch September 11, 2020 22:19
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