Skip to content

Conversation

jwood803
Copy link
Contributor

Fix for #1669.

@codecov
Copy link

codecov bot commented Feb 27, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@a0edc5c). Click here to learn what that means.
The diff coverage is 55.7%.

@@            Coverage Diff            @@
##             master    #2754   +/-   ##
=========================================
  Coverage          ?   71.66%           
=========================================
  Files             ?      807           
  Lines             ?   142374           
  Branches          ?    16120           
=========================================
  Hits              ?   102027           
  Misses            ?    35912           
  Partials          ?     4435
Flag Coverage Δ
#Debug 71.66% <55.7%> (?)
#production 67.9% <52.71%> (?)
#test 85.85% <92.3%> (?)
Impacted Files Coverage Δ
...rosoft.ML.Data/Scorers/PredictedLabelScorerBase.cs 82.02% <ø> (ø)
src/Microsoft.ML.Data/Dirty/PredictionUtils.cs 28.91% <ø> (ø)
...soft.ML.StandardLearners/Optimizer/SgdOptimizer.cs 0% <0%> (ø)
...andardLearners/Optimizer/DifferentiableFunction.cs 0% <0%> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs 63.84% <0%> (ø)
src/Microsoft.ML.TimeSeries/EigenUtils.cs 7.71% <0%> (ø)
....Data/Evaluators/MultiOutputRegressionEvaluator.cs 0.37% <0%> (ø)
...rosoft.ML.StandardLearners/Optimizer/LineSearch.cs 0% <0%> (ø)
src/Microsoft.ML.Sweeper/ISweeper.cs 65.11% <0%> (ø)
...rosoft.ML.Data/Transforms/LabelConvertTransform.cs 15.51% <0%> (ø)
... and 34 more

@TomFinley TomFinley added the code-sanitation Code consistency, maintainability, and best practices, moreso than any public API. label Feb 27, 2019
"Long parameter", "lp")]
[assembly: LoadableClass(typeof(FloatValueGenerator), typeof(FloatParamOptions), typeof(SignatureSweeperParameter),
"Float parameter", "fp")]
"float parameter", "fp")]
Copy link
Contributor

@TomFinley TomFinley Feb 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

float [](start = 5, length = 5)

I would probably avoid changing this particular one. This here is meant to be a user facing name, and we tend to capitalize these elsewhere. Though it is less important to get this exactly right, since it is part of our command line which is not part of the .NET API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I thought it was in reference to the Float type parameter that we were getting rid of.

// Extract the minimum, and the maximum value of the list of suggested sweeps.
// Positive lookahead splitting at the '-' character.
// It is used for the Float and Long param types.
// It is used for the float and Long param types.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

float [](start = 34, length = 5)

I think this is referring specifically to the FloatParamOptions, if you want to downcase it that's fine, but we should probably do the same with long here.

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good from my perspective, thank you for taking it on @jwood803 !! I had just one note about something related to the command line user name for something in a string that I think was deliberately called Float not float, as noted in the review. After that pretty minor change I think I'd be happy to approve. Thank you again for working on this!

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you for taking care of this @jwood803 ! using Float is now dead! (Well, soon will be, I suppose I should say.)

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@TomFinley TomFinley merged commit 1e234ef into dotnet:master Feb 28, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
code-sanitation Code consistency, maintainability, and best practices, moreso than any public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants