ReactiveBayes / RxInfer.jl

Julia package for automated Bayesian inference on a factor graph with reactive message passing
MIT License
254 stars 24 forks source link

Origin/dev allow failed #83

Closed Chengfeng-Jia closed 1 year ago

Chengfeng-Jia commented 1 year ago

For issue #59 , I change the inference function to allow user interruption, and still get the intermediate results. Beside, provide a test demo about how to use inference function with allow_failed argument.

review-notebook-app[bot] commented 1 year ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

albertpod commented 1 year ago

Thanks for the PR @Chengfeng-Jia. Good start; I think we can iterate through this PR a few times before we merge it to the master. I left a few comments on the code.

Have you noticed that FE doesn't compute when the inference function catches the error?

Also, we need to come up with a test for this functionality. Perhaps the inference function itself as well, we can think of it later :)

albertpod commented 1 year ago

Ah, yeah. Don't forget formatting.

Chengfeng-Jia commented 1 year ago

I will check these comments. I wrote a smoothing demo to test the interruption-allowed function. Maybe I can improve the demo so that it can trace the decline of FE and get the current inference result and FE value when interruption happens.

bvdmitri commented 1 year ago

Guys the PR at its current stage looks unnecessary over-complicated.

This feature does not require any new code. It is a simple re-arrangement of the existing lines. You should take the return statement + everything it depends on and simply put it outside of the try-catch block.

bvdmitri commented 1 year ago

Just to clarify a little bit what I mean, currently the code looks like:

actors = ...

try 
        fe_actor = ...
        ...
        posterior_values = ...
        fe_values = ...
        return ... 
catch 
        throw()
end

But it should be

actors = ...
fe_actor = ...

try 
        ...
catch 
        if !allow_failed
            throw()
        end
end

posterior_values = ...
fe_values = ...
return ...

That already solves the issue. Additional code can be written to save the error and to put it the InferenceResult.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 68.57% and project coverage change: -0.11 :warning:

Comparison is base (5de0236) 80.01% compared to head (6f10306) 79.91%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #83 +/- ## ========================================== - Coverage 80.01% 79.91% -0.11% ========================================== Files 10 10 Lines 1181 1200 +19 ========================================== + Hits 945 959 +14 - Misses 236 241 +5 ``` | [Impacted Files](https://codecov.io/gh/biaslab/RxInfer.jl/pull/83?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=biaslab) | Coverage Δ | | |---|---|---| | [src/inference.jl](https://codecov.io/gh/biaslab/RxInfer.jl/pull/83?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=biaslab#diff-c3JjL2luZmVyZW5jZS5qbA==) | `75.69% <68.57%> (-0.08%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=biaslab). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=biaslab)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

bvdmitri commented 1 year ago

@Chengfeng-Jia thanks! Its much better now! Lets wait for the #85 since it does implement the same check for the free energy actor as you did (with undef values).

The testing notebook should also be removed and instead of just throw the __inference_process_error should be used instead.

Chengfeng-Jia commented 1 year ago

@bvdmitri Thanks for your suggestion and patiently reviewing.

bvdmitri commented 1 year ago

@Chengfeng-Jia I merged the main branch and fixed the merge conflicts. I also prettified the code and added the documentation + tests. I think the PR is ready for review. Please check if you agree and mark it is as ready if so.

@albertpod @bartvanerp Do you think it makes sense to make catch_exception = true by default? Currently it false is the default.

Chengfeng-Jia commented 1 year ago

@bvdmitri Thanks for improving this code, I think it is ready for review.