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

ADP NaN bug! #3

Open
mnfaqiry opened this issue Sep 13, 2017 · 13 comments
Open

ADP NaN bug! #3

mnfaqiry opened this issue Sep 13, 2017 · 13 comments

Comments

@mnfaqiry
Copy link
Collaborator

mnfaqiry commented Sep 13, 2017

Hi @bpalmintier
At this point, we have our HEMS example running and working properly for DP ( using dpBI) and we are simulating some cases. For the ADP( using adpSBI) however, we are having this NaN issue that looks to be a toolbox bug based on the history of faLocalAvg.m below, if we have not misunderstood.

%% HISTORY
% ver     date    time       who     changes made
% ---  ---------- -----  ----------- ---------------------------------------
%  14  2017-04-26 22:55  BryanP      BUGFIX: fix to avoid excess NaN

When we run adpSBI for our HEMS problem, most of the time we get the following error.

>> HEMS_Main
Elapsed time is 0.000109 seconds.
Sampled Backward Induction ([0.1] ksamples/period)
    Creating empty post-decision value functions (LocalAvg)
    T=8 (terminal period): Done
    T=7:S........................................100
    T=6:S........................................**Error using faLocalAvg/build_func (line 255)
input data contains NAN values.

Error in FuncApprox/approx (line 201)
                obj.build_func();

Error in faLocalAvg/approx (line 207)
            out_vals = approx@FuncApprox(obj, out_pts);

Error in FindOptDecFromVfun (line 43)
    future_val_list = approx(vfun, vfun_state_list, vfun_approx_params{:});

Error in adpSBI (line 403)
    parfor next_pre_idx = 1:n_next_pre

Error in HEMS_Main (line 34)
results = adpSBI(HEMS_problem, sbi_opt);**

The sampling parameters we used for this particular case is below:

sbi_opt = {     'sbi_state_samples_per_time'            100    % Number of state samples per time period
                'sbi_decisions_per_sample'                      20    % Number of decision samples per state
                'sbi_uncertain_samples_per_post'            20   % Number of random/uncertainty samples per time, used for all decisions
                'vfun_approx'                           'LocalAvg'
                };

We would like your inputs on this issue.

@bpalmintier
Copy link
Collaborator

bpalmintier commented Sep 14, 2017 via email

@bpalmintier
Copy link
Collaborator

@mnfaqiry Seems you may have found a solution by typecasting to double can you either push that update or point me to the right place so I can include it in the main code?

@kdheepak
Copy link
Member

kdheepak commented Sep 24, 2017

I believe it was this line, but I do see it being used in a couple of other places namely here and here.

@hywu80
Copy link
Collaborator

hywu80 commented Sep 24, 2017 via email

@kdheepak
Copy link
Member

@hywu80 develop branch is "protected", you can push to a separate branch if you'd like. If you've already made the commit on develop, you can do the following.

# Create new branch
git checkout -b fix-for-kdtree-issue
# Push new branch to remote
git push -u origin fix-for-kdtree-issue

# Delete commit from develop
git checkout develop
git reset --hard origin/develop

Let me know if you have any questions.

kdheepak added a commit that referenced this issue Sep 24, 2017
@kdheepak
Copy link
Member

@hywu80 I moved your changes to a separate branch following the instructions above. I also fixed the NaN bug on develop. There were a couple of other changes in your file which I haven't added to develop yet. The diff for those changes are here.

@hywu80
Copy link
Collaborator

hywu80 commented Sep 24, 2017

Thanks @kdheepak. Since all changes were made by Li Wang. Can you add him into this repo. His github ID is wangli1030. He probably can explain why he changed those lines.

@bpalmintier
Copy link
Collaborator

bpalmintier commented Sep 25, 2017

Actually all of the remaining changes were ones made by @jbukenbe. I merged them into develop over the weekend. We need to keep them. This is why it is important to use the git tools directly, had the NAN change been made in a branch off of develop, they would have merged.

See comments I added to @kdheepak's link above

Agree that we should add Li, though!

@bpalmintier
Copy link
Collaborator

To clarify, as seen in comments I added to @kdheepak's link above, we need to revert the apparent changes made when copying in Li's version of the file, the base version in the repository had been updated with @jbukenbe's changes, that were clobbered by copying in Li's version.

@bpalmintier
Copy link
Collaborator

Also, taking a step back I'd like to better understand how casting to double is fixing any problems. By default all arrays in matlab will be doubles and all variables are arrays, so I don't understand how casting to double makes any changes at all. Could you explain further, ideally with an example of how this change makes any difference? One way to do so is to

@kdheepak
Copy link
Member

@bpalmintier The changes that @hywu80 provided are in a separate branch. develop only has the casting fix.

@kdheepak
Copy link
Member

So, @jbukenbe's changes should still be in the HEAD of origin/develop. They were in a separate branch so that we could review them. Sorry for the confusion, I should have explained more clearly.

@wangli1030
Copy link
Collaborator

@bpalmintier Yes. But actually, before casting it, the value in the matrix is single precision. I tried to debug the kdtree_build.cpp file, and when there is a NAN error, it reads some wired and random value. So I thought it may be the precision problem. First, I thought may be this matrix which passed into kdtree_build using some value in my problem setup, so I cast all variables in my code to double, but it is still single precision. Also, I tried to find out how does this matrix created, but it is really hard. Therefore, I just directly cast it in the source code.

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

No branches or pull requests

5 participants