39 refactor mljflux interface #92
Conversation
…nified MLJFlux.shape! and MLJFlux.build!
… required. Modified MMI.clean! accordingly
… required. Modified MMI.clean! accordingly. Now the likelihood type is automatically set by the inner constructor once the user picks either LaplaceRegression or LaplaceClassification
…liaTrustworthyAI/LaplaceRedux.jl into 39-refactor-mljflux-interface (tried amend)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Let's try the following:
- Use
glm_predictive_distributionto return actual predictive posterior. - Let
glm_predictive_distributionalways return normal distribution. - Add Distributions.jl as dep.
- In the
predict(la::Laplace)function ( ), to get first two moments, simply callmeanandstdon returnedDistribution.
glm_predictive_distribution(la, X) |>
dist -> (mean(dist), var(dist))
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
JuliaFormatter
src/mlj_flux.jl|353|
src/mlj_flux.jl|368|
src/mlj_flux.jl|370|
src/mlj_flux.jl|372|
src/mlj_flux.jl|378|
src/mlj_flux.jl|384|
src/mlj_flux.jl|387 col 1|
src/mlj_flux.jl|389|
src/mlj_flux.jl|394|
src/mlj_flux.jl|400|
src/mlj_flux.jl|406 col 1|
src/mlj_flux.jl|417|
src/mlj_flux.jl|423 col 1|
test/mlj_flux_interfacing.jl|97|
|
@pat-alt i will not work on this anymore without some kind of indication because i am confused and tired. i tried to change it back as it is was before (and adding the required fields as written here) https://github.com/FluxML/MLJFlux.jl/tree/01ad08ebc664f16d9509171866685c14d7bd6e99 but it doesn't work. |
OK! It looks like the package fails to precompile, so it's hard to tell if anything in the code itself is wrong. I would suggest the following:
Then tackle the tasks here, as discussed. Also, try to remember to sometimes apply the linter: using JuliaFormatter
JuliaFormatter.format(".") |
… solved one of the issue. the remaining two are still there
…ded y instead of categorical)
|
@MojiFarmanbar to see |
src/mlj_flux.jl
Outdated
| @@ -426,7 +422,7 @@ function MLJFlux.fit!( | |||
|
|
|||
| #return cache, report | |||
|
|
|||
| return (fitresult, report, cache) | |||
| return (fitresult, cache, report) | |||
There was a problem hiding this comment.
@pat-alt @MojiFarmanbar i have changed the position as we said. this have a negative effect during the testing since now i get (training_losses = (),) when i use the same order in mlj_flux_interfacing ( fitresult, cache, _report = MLJBase.fit(model, 0, X, y)) . the order seems correct ( https://github.com/FluxML/MLJFlux.jl/blob/07e01b7f41bc3dd870dd08c7c97114cc45f91f5e/src/mlj_model_interface.jl#L149)
There was a problem hiding this comment.
@Rockdeldiablo , then the issue was the order of arguments, right? did you try to see if the test cases will pass? it seems it fails because of _report.
There was a problem hiding this comment.
@MojiFarmanbar the content does not correspond. cache is not empty despite i didn't use it at all. in third position i pass cache as () but i get the training loss. in second position i pass the training loss through report, but i get a list of numbers that looks like weights. it mess with the update function that does not work as it should
There was a problem hiding this comment.
@Rockdeldiablo, did you check your environment is correctly set? i suppose your package is in dev mode that you could see your changes immediately?
There was a problem hiding this comment.
@Rockdeldiablo, did you check your environment is correctly set? i suppose your package is in dev mode that you could see your changes immediately?
yes. i have updated the env, moved everything to the new api and now the issue with report is solved. there is however another issue in the classification task with the history of the loss that grows instead of going down
There was a problem hiding this comment.
@MojiFarmanbar i think it's better if i close this pull request since it's a mess and perhaps open a different one
There was a problem hiding this comment.
@Rockdeldiablo, maybe we can keep this PR still open and you can create a new PR. I understand that you might do a lot of experimentation but make sure you don't loose the track of them, try to isolate your experiments from each other otherwise they might collide and get confusing.
from above, what I would learn:
- we need to make sure the environment is updated with the changes in the code otherwise we get inconsistent results.
- Isolating the experiments
do you agree with me?
There was a problem hiding this comment.
@Rockdeldiablo, maybe we can keep this PR still open and you can create a new PR. I understand that you might do a lot of experimentation but make sure you don't loose the track of them, try to isolate your experiments from each other otherwise they might collide and get confusing. from above, what I would learn:
- we need to make sure the environment is updated with the changes in the code otherwise we get inconsistent results.
- Isolating the experiments
do you agree with me?
yes, but i do experiments because it's not clear how should i proceed with mljflux.update. it needs the old_history to work and the only way to pass it is through cache, but patrick told me to not use it. Maybe some code reviews with some code tips could help me find the errors quickly and move ahead. besides, the original project was about adding conformalized bayes support to conformalprediction.jl , not how to add to mljbase support to laplaceredux.
There was a problem hiding this comment.
I'll take a look at this tomorrow morning
There was a problem hiding this comment.
besides, the original project was about adding conformalized bayes support to conformalprediction.jl , not how to add to mljbase support to laplaceredux.
You're free to use any other library for Bayesian models, it doesn't need to be LaplaceRedux. But ConformalPrediction interfaces MLJ, that's a conscious design choice, so support for adding conformalized Bayes was always going to involve working with MLJ.
… report" This reverts commit 0bf4ea8.
… updated mljflux to 0.5.1
…e on how train_epoch handle the crossentropy function since the loss rise instead of going down
|
I think this is actually pretty close now 👍🏽 collecting a few observations below as I'm working on the code:
Edit: more specifically, it appears that calling
julia> MLJBase.update(model, 2, chain, cache, X, y)
[ Info: Loss is 12.27
[ Info: Loss is 0.4794
[ Info: Loss is 0.4555
[ Info: Loss is 0.4437
[ Info: Loss is 2.295
[ Info: Loss is 0.455
[ Info: Loss is 0.4554
[ Info: Loss is 0.4562
[ Info: Loss is 170900.0
[ Info: Loss is 457.6
[ Info: Loss is 43.38
[ Info: Loss is 1.24
[ Info: Loss is 0.9924
[ Info: From train
Chain(Dense(4 => 16, relu), Dense(16 => 8, relu), Dense(8 => 1))
[ Info: From fitresult
Chain(Dense(4 => 16, relu), Dense(16 => 8, relu), Dense(8 => 1))
[ Info: From fitresult
(Chain(Dense(4 => 16, relu), Dense(16 => 8, relu), Dense(8 => 1)), LaplaceRegression(builder = MLP(hidden = (16, 8), …), …)) |
|
@Rockdeldiablo let me take over here for a moment, think it's just a few more minor issues that I can hopefully fix. Then that gives you an opportunity to finish the remaining tasks on #97 😃 |
| P₀=model.P₀, | ||
| ) | ||
| verbose_laplace = false | ||
| if !isa(chain, AbstractLaplace) |
There was a problem hiding this comment.
@pat-alt why this check? when does chain turns in a laplace object to require this check?
There was a problem hiding this comment.
Hey @Rockdeldiablo, please see my comment below, that should clarify this.
Edit: There might be a cleaner way to go about this that avoids the conditional, but I think I have convinced myself that returning the whole Laplace object is the way to go, because it contains the chain. The downside is that this may lead to unexpected behavior in the future if MLJFlux adds downstream functionality foo(chain) for the chain returned by train, that the Laplace object does not support. But since it contains the chain we can always address such cases swiftly foo(la) = foo(la.model) (where la.model is the chain).
|
@Rockdeldiablo I've added the following changes now to make things work without having to overload the
Now that tests are passing, there are a few more things to do (possibly in a new issue + PR) if you like.
For now, feel free to to focus on the other PR, just ping me and @MojiFarmanbar when you come back to this one. I need to move on to other things for now. |
…mitted just to avoid stashing
|
Let's just move the pending tasks above into new issues and then merge this one. |
ahh i just saw this message. how do i move the pending tasks in new issues? |
i have tried to use the new @mlj_model macro.
there are some choices that i am not sure if they are right.
First, the @mlj_model macro allows to define constraints directly in the struct, so there is no need for a clean! function, but if you prefer the older method i can go back.
Second, the mljmodelinterface quick guide tells that in the structs there need to be only hyperparameters, so i removed the builder field and now that chain has to be passed directly to fit!. It seems also logical since one use may want to experiment with different flux models without having to redefine the hyperparameters tied to the Laplace wrapper. However i am not sure that adding an argument to the fit! function is the correct choice since all the examples models do not do it.
third, i removed the two shape and build functions since they are at most a generic utility for the user that is not necessarily connected to the Laplaceredux package.
It works (at least on my pc....), but i am not sure if it respect what MLJ wants and why these automatic checks complains so much. Is it because i didn't add the Project.toml and manifest.toml files?