search

IQVIA-ML / LightGBM.jl

Julia FFI interface to Microsoft's LightGBM package
Other
93 stars 10 forks source link

Add MLJ compliant docstrings #130

Open josephsdavid opened 1 year ago

josephsdavid commented 1 year ago

In service of #913, as documented here !

yaxxie commented 1 year ago

Hi @josephsdavid Thanks for the contribution!

A couple of remarks; 1) Please avoid simply reformatting code. This makes diffs harder to read and muddies the purpose of the contribution 2) Please fill in the description to the PR. For example, a link to the documentation about "MLJ docstrings" would be useful to the reader. 3) Please also don't forget to add yourself to the contributors list CONTRIBUTORS.md :slightly_smiling_face:

josephsdavid commented 1 year ago
  1. Please fill in the description to the PR. For example, a link to the documentation about "MLJ docstrings" would be useful to the reader.

hah i was so excited to have all the parameters documented i missed the other pieces of work 😓

josephsdavid commented 1 year ago

@josephsdavid Thanks for this mammoth effort. 🦣

I don't see sections "Fitted parameters" or "Report", which are required.

Given the fact that all the models have a lot of hyper-parameters in common, I wonder if you would consider, for easier maintenance, interpolating a string constant for the common ones?

I've looked over the first docstring for now. Please ping me when you've addressed my comments and I'll review the others too.

Will do! going to go over more closely over the weekend :)