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

Incorrect d$FN vector in JM_BR.R #29

Open
vshekar opened this issue Jan 5, 2017 · 9 comments
Open

Incorrect d$FN vector in JM_BR.R #29

vshekar opened this issue Jan 5, 2017 · 9 comments
Assignees
Labels

Comments

@vshekar
Copy link
Collaborator

vshekar commented Jan 5, 2017

In the JM model specifically this function the d$FN vector seems to be incorrect. For example, dataset SYS1 has 136 points which means d$FN should start from 137 but starts from 136 which is not right.

A corresponding function in GO model also uses the same variable and correctly starts from 137 for SYS1.

This has lead to the incorrect calculation of the number of predictions the model can make. JM makes 1 more prediction than it's supposed to.

@vshekar vshekar added the bug label Jan 5, 2017
@vshekar
Copy link
Collaborator Author

vshekar commented Jan 6, 2017

Fixed the issue. But not sure if correct. Changed the floor function to round here : RunModels.R.

Can someone explain to me or add comments as to what the if else statement does? The else block is entered only for the JM model and incorrectly calculates the lower_pred_bound to be 136 rather than 137 when the floor function is used.

I'll close the bug once this is explained.

@arhik
Copy link
Contributor

arhik commented Jan 13, 2017

I didn't quite get the part from the first comment

n the JM model specifically this function the d$FN vector seems to be incorrect. For example, dataset SYS1 has 136 points which means d$FN should start from 137 but starts from 136 which is not right.

@vshekar
Copy link
Collaborator Author

vshekar commented Jan 13, 2017 via email

@arhik
Copy link
Contributor

arhik commented Jan 13, 2017

MVF_Inv function is just the inverse of the data points it receives. So any extra data point or missing data point is purely mistake else where data comes from(And I suspect its run models mishandling the data points). The functions are fine. And I want it to be purely mathematical from 'Model contribution' point of view. So any changes to it will confuse contributors. Also data returned from this function should be handled elsewhere as needed because this is a mathematical modeling file. I hope I am making sense.

This means d$FN should be [137, 138, 139, ..., 144] for both models. But I found that in the JM model d$FN = [136, 137,..., 143]. This error was cascading into other functions which lead to JM predicting the wrong number of points. Hope that makes sense.

The missing data point is clearly the data handling issue prior to calling the MVF_Inv function. The extra logic in the MVF_Inv is a bit scary from contributors perspective.

@arhik
Copy link
Contributor

arhik commented Jan 13, 2017

Let me try with just the rounding part in runModels. I will let you know if any thing is abnormal.

@vshekar
Copy link
Collaborator Author

vshekar commented Jan 13, 2017

You are right, RunModels is mishandling the data which was what my 2nd comment in this thread talks about. The extra logic to replace Nan values was to keep everything else from breaking, which I had added before raising this issue: #28. In the issue I mentioned that we can remove the logic once I figure out what RunModels does.

@arhik
Copy link
Contributor

arhik commented Jan 13, 2017

Regarding #28: Sorry I was busy then.

@arhik
Copy link
Contributor

arhik commented Jan 13, 2017

Shekar can you point me to the bug or commit you fixed by using max to avoid nan values.

@vshekar
Copy link
Collaborator Author

vshekar commented Jan 13, 2017

There was no specific issue raised for it as far as I know. But if you asked the model to predict more points than it can and tried to plot it, it would show an error on the browser. You can probably recreate it by removing the max logic and set the finite model param to False. The commit is this one: f1226d9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants