Reduce sigdigits in parameter range display #948

[adarshpalaskar1]

This commit addresses issue #948 by updating the display of parameter ranges to include reduced significant digits.

[ablaom]

Thanks @adarshpalaskar1 for this contribution. Looks like you need to update a test.

[adarshpalaskar1]

Sorry, I didn't run the tests locally before creating the pull request.
I saw the logs and found this test is failing:

range(Int, :junk, lower=1, upper=10)

Log:

  Expression: String(take!(io)) == "NumericRange(1 ≤ junk ≤ 10; origin=5.5, unit=4.5)"
   Evaluated: "NumericRange(1.0 ≤ junk ≤ 10.0; origin=5.5, unit=4.5)" == "NumericRange(1 ≤ junk ≤ 10; origin=5.5, unit=4.5)"

Should I update the test, or is the updated code incorrect?

[ablaom]

I think your changes are good. You just need to update the invalidated test. How about you improve the test by using lower and upper values with more that 4 sigdigits and verifying (with a variation on the existing test) that extra digits get truncated?

[ablaom]

Okay, I see. In the <:Integer case, we don't want to apply rounding at all. Sorry I missed that. So you need to add some logic.

[adarshpalaskar1]

Okay, I have added a check before rounding off. I faced some errors while executing the tests locally so tried some test cases manually.

Update: I was able to run the tests locally, and all of them passed.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f3da249) 87.66% compared to head (02af6e0) 88.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #958      +/-   ##
==========================================
+ Coverage   87.66%   88.00%   +0.33%     
==========================================
  Files          28       28              
  Lines        2433     2542     +109     
==========================================
+ Hits         2133     2237     +104     
- Misses        300      305       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ablaom]

Can we please add at test here:

MLJBase.jl/test/hyperparam/one_dimensional_ranges.jl

Line 122 in f3da249

r2 = range(Char, :junk, values=['c', 'd'])

?

For example, something for r3= range(Float64, :junk, lower=3.14159, upper=6.283185)

[adarshpalaskar1]

Can we please add at test here:

MLJBase.jl/test/hyperparam/one_dimensional_ranges.jl

Line 122 in f3da249

r2 = range(Char, :junk, values=['c', 'd'])

?
For example, something for r3= range(Float64, :junk, lower=3.14159, upper=6.283185)

Sure, I have added the test and executed it locally. (commit 02af6e0)

julia> include("test\\hyperparam\\one_dimensional_ranges.jl")
Test Summary: | Pass  Total  Time
constructors  |   36     36  1.7s
Test Summary:                            | Pass  Total  Time
range constructors for nested parameters |    1      1  0.6s
Test Summary:       | Pass  Total  Time
Test Summary: | Pass  Total  Time
coverage      |    3      3  0.2s
true

[ablaom]

Perfect.

Thanks @adarshpalaskar1 for your valuable contribution.

[adarshpalaskar1]

Thank you @ablaom for helping me through my beginner doubts; this was my first contribution. If there's anything specific I should keep in mind for future contributions, and if there are more issues where I can help, please point me in the right direction.

[ablaom]

Maybe you'd like to take a stab at JuliaAI/MLJDecisionTreeInterface.jl#23 ?

[ablaom]

You can find some decision tree examples at https://alan-turing-institute.github.io/MLJ.jl/dev/models/DecisionTreeClassifier_DecisionTree/#Examples .

Once we have that, you'd be in good shape to improve integration with TreePlots.jl.

[ablaom]

Congratulations, by the way, on your first contribution 🎈

[adarshpalaskar1]

You can find some decision tree examples at https://alan-turing-institute.github.io/MLJ.jl/dev/models/DecisionTreeClassifier_DecisionTree/#Examples .

Once we have that, you'd be in good shape to improve integration with TreePlots.jl.

Thank you! I'll check out the decision tree examples and proceed.