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

Critical bug in uf_addmarginal() #67

Closed
olafdimigen opened this issue Apr 4, 2019 · 3 comments
Closed

Critical bug in uf_addmarginal() #67

olafdimigen opened this issue Apr 4, 2019 · 3 comments
Assignees
Labels

Comments

@olafdimigen
Copy link
Member

olafdimigen commented Apr 4, 2019

Function uf_addmarginal produces error with the attached test dataset.

Most likely because either of these reasons:

  • in the model, a rather large number of event types (e.g. {S 11}, {S 12}, {S 13},...) is grouped, e.g. for the intercept
  • more likely: model contains interaction term between categorical variables

Unfortunately, the code at the end of uf_addmarginal is quite obfuscated. Script and data to reproduce bug are found here:
https://www.dropbox.com/s/6cmy9t4693rzt4o/bugreport_addmarginal.zip?dl=0

The ZIP file contains:

  • bugreport_addmarginal.mat = test data containing "condensed" model output
  • github_bugreport_addmarginal = test script which reproduces the error
  • uf_addmarginal_bugreport = version of uf_addmarginal() with improved comments and the lines producing the bug highlighted
@behinger behinger self-assigned this Apr 4, 2019
@behinger behinger added the bug label Apr 4, 2019
@behinger
Copy link
Member

behinger commented Apr 4, 2019

One design decision I regret is to keep multiple events as cell arrays instead of combining them in some way... now it bites me. This is due to multiple events types.

I fixed it now, it is a bit opaque because one has to cycle through the cell arrays manually. Thanks for the comments, I added some of my own to make the last part more obvious whats going on.

There is one thing that worries me: I had

        if length(e_Idx) == 1
            continue 
        end

Included, but I really don't know anymore why. It intuitively does not make any sense to me. I will write a unittest and check it out in more detail

@olafdimigen
Copy link
Member Author

Yep, could not decipher the purpose of that either. No obvious reason to exclude/skip events that only occur once in unfold.param.event.

@behinger
Copy link
Member

behinger commented Apr 4, 2019

ok unittests work now. Thanks for the nice elaborate report!

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