Direct mlj interface #126
Direct mlj interface #126
Conversation
fixed a small mistake in the optimiser
uhm i have removed my definition of |
|
@pat-alt how would you find the definition of https://github.com/JuliaAI/MLJModelInterface.jl/blob/03ccc2dbdc8a87b869fad27bf550f62f8a26c005/src/equality.jl#L7 i can't find it anymore. |
That's a built-in function: I'll have a look at the issue with |
src/direct_mlj.jl
Outdated
| - values that are not of `MLJType` are "equal" if they are `==`. | ||
|
|
||
| In the special case of a "deep" property, "equal" has a different | ||
| meaning; see [`MMI.StatTraits.deep_properties`](@ref)) for details. |
There was a problem hiding this comment.
@pasq-cat the problem was that Documenter couldn't find this reference without the namespace.
There was a problem hiding this comment.
It's a Documenter.jl thing: https://documenter.juliadocs.org/stable/man/syntax/#@ref-link
There was a problem hiding this comment.
and indeed for cross-referencing, yes
| sigma_scaling, rescale_stddev | ||
| sigma_scaling, | ||
| rescale_stddev | ||
|
|
There was a problem hiding this comment.
@pasq-cat I have removed the include("mlj_flux.jl") here since we no longer need it? Let's then also remove that file altogether
There was a problem hiding this comment.
@pat-alt i have seen you have implemented an interface with mljflux in jointenergymodels.jl . does it work well without overloading any private method?
There was a problem hiding this comment.
No it doesn't, same issues as here I'm afraid, so the MLJFlux dependency there is currently upper bounded to v0.5. I think we'll have to use the same approach as here eventually (directly interfacing MLJ through MMI).
There was a problem hiding this comment.
i was thinking about this issue and i was wondering if we can't just take the function shape and build and use it with the mlj interface, without passing through mljflux.
shape depends only on the data, build is just adding an initial and final layer. can't we just write an if statement in the fit function where if model.model=nothing -> use shape and build to set a simple mlp?
There was a problem hiding this comment.
hmm possibly! Let's discuss it next Tuesday?
|
@pasq-cat I have removed the |
There was a problem hiding this comment.
If this is no longer needed, let's removed it (ipynb always clogs up the repo).
Alternatively, could also turn into a qmd and make it part of the docs if relevant.
There was a problem hiding this comment.
there are a couple of issues before we merge this pr.
- do you want to merge this one before the one for verbose->verbosity switch? change verbose::Bool with verbosity::Int #127 . if so we will have to modify the interface again before asking ablaom to review it.
- I would keep the ipynb for now if you are ok with it. once the interface is complete and ablaom has reviewed it i will transform it in a .qmd for the documentation.
- the patch coverage is still under the threshold. When i try to raise it further, i fail. It seems that the tests never hit the missing lines, which is weird and worrying.
There was a problem hiding this comment.
Regarding 1, good point, we should probably wait.
Regarding 2, yes, that's fine.
Regarding 3, are you able to inspect the missing coverage on codecov or do you not have access rights?
There was a problem hiding this comment.
@pat-alt ops, i tried to answer via email but it seems I didn't answer under the right comment. I will post the text here again:
i can see the codecov lines, the problem is that when i tried to set a test that would cover those lines it didn't work. Theoretically when the model.model change to a different chain the last condition should be called but it wasn't.
i will try again next week because monday i have an important test.
|
@pat-alt sorry but looks like I can't do better than this. Looking at the COdecov report it seems that the "return false" lines in ps: i added a simple builder for when the user leave nothing in the field model. |
solve this #121