Skip to content

Commit

Permalink
Complete doc_string for fitted_params()
Browse files Browse the repository at this point in the history
  • Loading branch information
@roland-KA
roland-KA committed Mar 31, 2022
1 parent 7f33de3 commit 70507b9
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/OneRule_MLJ.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ This classifier has no hyper-parameters.
The fields of `fitted_params(mach)` are:
- `tree`: the tree (a `OneTree`) returned by the core OneTree.jl algorithm
- `a_target_element`: for internal use; it's a target element of the training data used to
transfer `levels`-information to `predict`

This comment has been minimized.

Sign in to view

Copy link
@ablaom

ablaom Mar 31, 2022

If it's only for internal use, you could just drop it from fitted_params. However, you may want to think about passing, either here or in the report, any encoding which would allow one to plot an appropriately labelled tree, as per discussion at https://github.com/bensadeghi/DecisionTree.jl/issues/147.

This comment has been minimized.

Sign in to view

Copy link
@roland-KA

roland-KA Apr 1, 2022

Author Owner

I'm passing the information about class labels and feature labels in report (classes_seen and features). In an earlier version I returned the same information also with fitted_params. But then I thought, that these values are not really "fitted parameters", so I decided to return it only in report.

What is your opinion about that? Are there any conventions about what fitted_params and report should do?

This comment has been minimized.

Sign in to view

Copy link
@roland-KA

roland-KA Apr 1, 2022

Author Owner

As I need the information from a_target_element (namely its levels), I've changed the parameter so that it returns all levels that were present in the training data of the target (and its name is now 'all_classes'). That might also be useful for external use.

This comment has been minimized.

Sign in to view

Copy link
@ablaom

ablaom Apr 3, 2022

But then I thought, that these values are not really "fitted parameters", so I decided to return it only in report.

That's perfectly correct. See also here: https://alan-turing-institute.github.io/MLJ.jl/dev/quick_start_guide_to_adding_models/#Summary-of-user-interface-points-(or,-What-to-put-where?)

As I need the information from a_target_element (namely its levels), I've changed the parameter so that it returns all levels that were present in the training data of the target (and its name is now 'all_classes'). That might also be useful for external use.

👍🏾

This comment has been minimized.

Sign in to view

Copy link
@ablaom

ablaom Apr 3, 2022 •
edited
Loading

Re encoding, if you include that in the fitted_params, see JuliaAI/MLJDecisionTreeInterface.jl#19 .

This comment has been minimized.

Sign in to view

Copy link
@roland-KA

roland-KA Apr 5, 2022

Author Owner

Thanks for the information about "user interface points"! So I was on the correct path 🤓.

As I don't apply an "encoding" of labels (like DecisionTree does), I think it's not necessary to return one in this context. But the issue you opened (#19) is a good idea. I've already stumbled over this and wondered what the purpose of that structure could be 😊.
# Report
Expand Down

0 comments on commit 70507b9

Please sign in to comment.