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

NaN parameters in dict_list change @onlyif behaviour #353

Open
lhupe opened this issue Jul 27, 2022 · 1 comment
Open

NaN parameters in dict_list change @onlyif behaviour #353

lhupe opened this issue Jul 27, 2022 · 1 comment
Labels
bug Something isn't working running-listing Functionality for running and listing simulation runs

Comments

@lhupe
Copy link
Contributor

lhupe commented Jul 27, 2022

By some spooky action at a distance, introducing a NaN-valued parameter into a dict_list changes the behaviour of @onlyif restrictions, even if they shouldn't depend on that parameter at all:

Minimal Working Example
Versions: DrWatson v2.9.1 and Julia v1.7.1

dict_list(Dict(
    :a => Inf,
    :b => [@onlyif(:c, 1), 2],
    :c => [true, false]
))

returns

3-element Vector{Dict{Symbol, Real}}:
 Dict(:a => Inf, :b => 1, :c => true)
 Dict(:a => Inf, :b => 2, :c => true)
 Dict(:a => Inf, :b => 2, :c => false)

while

dict_list(Dict(
    :a => NaN,
    :b => [@onlyif(:c, 1), 2],
    :c => [true, false]
))

returns

4-element Vector{Dict{Symbol, Real}}:
 Dict(:a => NaN, :b => 1, :c => true)
 Dict(:a => NaN, :b => 2, :c => true)
 Dict(:a => NaN, :c => false)
 Dict(:a => NaN, :b => 2, :c => false)

I've had a quick look at the dict_list code and spotted the problem: the last pass on the trial parameter combinations filters out all combinations that are subsets of other parameter combinations. Normally, this should remove the extra entry in the MWE since it only differs from the last entry by the missing dependent parameter, however, since NaN != NaN, Dict(:a => NaN, :c => false) is technically not a subset of Dict(:a => NaN, :b => 2, :c => false) and the comparison in dict_list.jl#92 fails.

My initial idea for a fix is to replace the trials[k] == _trials[k] with trials[k] == _trials[k] || trials[k] === _trials[k], since the === catches the NaNs, but the == is still necessary to catch mutable objects of equal value. Note that I have not thought this through at all, there may well be some edge case which breaks this.

@Datseris Datseris added bug Something isn't working running-listing Functionality for running and listing simulation runs labels Jul 28, 2022
@Datseris
Copy link
Member

sounds good, maybe open a PR with the === check? Also cc @JonasIsensee maybe he knows more. actually, I don't remember anymore who contribute the @onlyif macro...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working running-listing Functionality for running and listing simulation runs
Projects
None yet
Development

No branches or pull requests

2 participants