-
Notifications
You must be signed in to change notification settings - Fork 4
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pasq-cat the problem was that Documenter couldn't find this reference without the namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a Documenter.jl thing: https://documenter.juliadocs.org/stable/man/syntax/#@ref-link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and indeed for cross-referencing, yes
sigma_scaling, rescale_stddev | ||
sigma_scaling, | ||
rescale_stddev | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm possibly! Let's discuss it next Tuesday?
@pasq-cat I have removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @pasq-cat 👏🏽 thanks for being so persistent with this, it's really much appreciated!
Only two minor comments for you to see, but I'll already approve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @pasq-cat thanks again!
solve this #121