Direct mlj interface #126

pasq-cat · 2024-10-15T12:32:47Z

solve this #121

fixed a small mistake in the optimiser

…jflux.

…dels

pasq-cat · 2024-10-18T10:01:20Z

@pat-alt everything works but i fail to improve the patch coverage further.

pat-alt · 2024-10-21T06:29:10Z

@pat-alt everything works but i fail to improve the patch coverage further.

Great, nice work! The docs are failing due to this error:

Error: Cannot resolve @ref for md"deep_properties" in src/reference.md.

No docstring found in doc for binding LaplaceRedux.deep_properties.
No docstring found in doc for binding Main.deep_properties.
CheckDocument: running document checks.
Populate: populating indices.

Is this really related to MLJ rules? Seems like there's just a missing docstring for deep_properties.

pasq-cat · 2024-10-21T10:54:23Z

@pat-alt everything works but i fail to improve the patch coverage further.

Great, nice work! The docs are failing due to this error:

Error: Cannot resolve @ref for md"deep_properties" in src/reference.md.

No docstring found in doc for binding LaplaceRedux.deep_properties.

No docstring found in doc for binding Main.deep_properties.
CheckDocument: running document checks.
Populate: populating indices.

Is this really related to MLJ rules? Seems like there's just a missing docstring for deep_properties.

uhm i have removed my definition of deep_properties and _equal_to_depth_one since they are not needed (probably a remnant of some older test i made), and i still get the same error, probably because the docstring is missing in the MLJ definitions

pasq-cat · 2024-10-21T11:07:38Z

@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.

pat-alt · 2024-10-22T10:59:45Z

@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:

julia> Base.isdefined
isdefined (built-in function)

I'll have a look at the issue with deep_properties

…space

pat-alt · 2024-10-22T11:09:53Z

src/direct_mlj.jl

+- 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.


@pasq-cat the problem was that Documenter couldn't find this reference without the namespace.

@pat-alt can you explain me how to use that "@ref" ? what does it mean? i guess it is used for crossreferencing but i have never used it

It's a Documenter.jl thing: https://documenter.juliadocs.org/stable/man/syntax/#@ref-link

and indeed for cross-referencing, yes

pat-alt · 2024-10-22T11:14:05Z

src/LaplaceRedux.jl

-    sigma_scaling, rescale_stddev
+    sigma_scaling,
+    rescale_stddev
+


@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

@pat-alt i have seen you have implemented an interface with mljflux in jointenergymodels.jl . does it work well without overloading any private method?

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).

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?

hmm possibly! Let's discuss it next Tuesday?

pat-alt · 2024-10-23T08:09:00Z

@pasq-cat I have removed the @ref in the MMI.is_same_except docstring, because the deep_properties is part of MLJBase and cannot be accessed by Documenter.jl unless we use DocumenterInterLinks.jl. This looks a little more involved than what I currently have time for, so I'll just open a new issue for this. Feel free to explore this, would eventually be nice to use DocumenterInterLinks.jl to connect different documentations of Taija (but also feel free not to 😅 )

pat-alt

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.

pat-alt · 2024-10-23T09:51:54Z

dev/notebooks/mlj-interfacing/direct_mlj.ipynb

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 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.

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?

@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.

src/baselaplace/predicting.jl

pasq-cat · 2024-10-30T17:25:34Z

@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 _equal_flux_chain are not hit even when the two flux models are different but the tests seem to work as intended. if you want to take a look... otherwise, we merge and then ablaom will tell us if there is something wrong.

ps: i added a simple builder for when the user leave nothing in the field model.

pat-alt

LGTM @pasq-cat thanks again!

pasq-cat added 30 commits September 12, 2024 20:27

new first commit

51ed526

various stuff

e8e96d1

fixes

255cf19

changes

35ac2d8

there is still a problem with the classifier

44469a2

almost fixed

5315264

works but i have to fix the hyperparameters

f6e7a00

question on parameters....

d7c4f7b

there is some problem with the one hot encoding

bbab460

fixed error in univariatefinite

8af38ae

performance improvement

fe19d4d

JuliaFormatter

d809afb

juliaformatter+docstrings

33d84f5

removed predict_proba and ret_Distr from the struct

9731297

mlj docstring in progress

f70d239

ah fixed constant , added prototype for regression

80c6553

small stuff here and there in the docstring plus

d1c895c

fixed a small mistake in the optimiser

still writing this long ass docstring

19ffa16

added fit_params functions

de0bd91

switched to customized loop

87df85f

fixed error in custom loop

24459a1

various fixes

0e2ca03

added reformat. must updated again the doc string....

841d5eb

work on the docstring and then made it in a module

de784f1

fixed uuid, made test file.for direct_mlj. shut down the tests for ml…

b7a99f6

…jflux.

added tests. should be good....

c44b8d8

added mlj to the dependency in test

b762185

prep for update + added mljmodelinterface to doc env

ced3da0

changed the loop so that it nows uses optimisers from optimisers.jl

b700f85

started joining the functions in a single common function for both mo…

da6fc76

…dels

uhmmmmmm

573ffd8

fixed _isdefined

db14b84

pat-alt added 2 commits October 22, 2024 13:07

trying to fix docs issue and no longer importing MLJ nor MLJBase name…

82c5714

…space

formatting

7202013

pat-alt reviewed Oct 22, 2024

View reviewed changes

removing mlj_flux

a05e25f

pat-alt reviewed Oct 22, 2024

View reviewed changes

pat-alt added 2 commits October 22, 2024 15:11

fixed issues

05df2e1

removing reference to deep_propertier

02abec2

pat-alt mentioned this pull request Oct 23, 2024

Add external reference for MLJBase.deep_properties #128

Open

hadn't saved file

374aca5

pat-alt self-requested a review October 23, 2024 09:50

pat-alt approved these changes Oct 23, 2024

View reviewed changes

pasq-cat added 7 commits October 29, 2024 22:23

Merge branch 'main' into local_direct_mlj

06343bd

added default mlp

59917f8

reducing number of epochs and trying to extende patch coverage

9da5d7d

removed the else because it seems to have no role.

503763d

ops forgot to remove the comment

e78e9b8

various change in the documentation

6a5f26f

ufffffffffffffffffffff

12f2584

pasq-cat requested a review from pat-alt October 30, 2024 17:26

pat-alt approved these changes Nov 5, 2024

View reviewed changes

pasq-cat merged commit 5f66429 into main Nov 7, 2024
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Direct mlj interface #126

Direct mlj interface #126

pasq-cat commented Oct 15, 2024

pasq-cat commented Oct 18, 2024

pat-alt commented Oct 21, 2024

pasq-cat commented Oct 21, 2024

pasq-cat commented Oct 21, 2024

pat-alt commented Oct 22, 2024

pat-alt Oct 22, 2024 •

edited

Loading

pasq-cat Oct 22, 2024

pat-alt Oct 22, 2024

pat-alt Oct 22, 2024

pat-alt Oct 22, 2024

pasq-cat Oct 23, 2024 •

edited

Loading

pat-alt Oct 23, 2024

pasq-cat Oct 23, 2024

pat-alt Oct 23, 2024

pat-alt commented Oct 23, 2024

pat-alt left a comment

pat-alt Oct 23, 2024

pasq-cat Oct 23, 2024 •

edited

Loading

pat-alt Oct 23, 2024

pasq-cat Oct 24, 2024

pasq-cat commented Oct 30, 2024

pat-alt left a comment

Direct mlj interface #126

Direct mlj interface #126

Conversation

pasq-cat commented Oct 15, 2024

pasq-cat commented Oct 18, 2024

pat-alt commented Oct 21, 2024

pasq-cat commented Oct 21, 2024

pasq-cat commented Oct 21, 2024

pat-alt commented Oct 22, 2024

pat-alt Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pasq-cat Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pat-alt commented Oct 23, 2024

pat-alt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pasq-cat Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pasq-cat commented Oct 30, 2024

pat-alt left a comment

Choose a reason for hiding this comment

pat-alt Oct 22, 2024 •

edited

Loading

pasq-cat Oct 23, 2024 •

edited

Loading

pasq-cat Oct 23, 2024 •

edited

Loading