Machinelearning: CpuMath Enhancement: Fix naming of non-constant privates in perf tests for hardware intrinsics

[briancylui]

Style changes needed to solve part of https://github.com/dotnet/machinelearning/issues/823

Details

• In test\Microsoft.ML.CpuMath.PerformanceTests\PerformanceTests.cs, regarding the line private float[] src, dst, original, src1, src2;, all these non-constant privates should be prefixed with _ but it can be updated in the future PR.

[jwood803]

Hey, @briancylui! Just taking a look at this issue and it looks like those variables are protected. Do they still require the _ prefix?

https://github.com/dotnet/machinelearning/blob/8ac5ce85cab0c82c536a3e98696b37d6df1405cd/test/Microsoft.ML.CpuMath.PerformanceTests/PerformanceTests.cs#L28

[briancylui]

Hey, @jwood803! Thanks for your inquiry. You are right in that the _ prefix was suggested when these variables were private rather than protected. @safern, since you are more familiar with the C# naming convention and you made the suggestion earlier, may you please take a look at whether the _ prefix is still needed for those non-constant protected variables? Thanks!

[safern]

Protected variables shouldn't have _ prefix. I just suggested it for the private fields.

For example I see:
https://github.com/dotnet/machinelearning/blob/345a5c2f2f16087f3aa7f606799b812ac9fa38a5/test/Microsoft.ML.CpuMath.PerformanceTests/PerformanceTests.cs#L31

in that file. Might be worth just scanning files that define private fields and make sure the align with the naming conventions.

[jwood803]

Thanks, @safern! I'll clean that and any other private fields I see in the file. 😄

[safern]

And if you could scan other files related to CpuMath that would be awesome 😄

Thanks for helping doing this!

[jwood803]

I'll be glad to! 😄

Thanks for the help on this!

[jwood803]

@safern, I think the one you pointed out was the only instance that needed updating. However, I did see some private const fields, but I'm unsure of what the naming convention is for them.

https://github.com/dotnet/machinelearning/blob/8ac5ce85cab0c82c536a3e98696b37d6df1405cd/test/Microsoft.ML.CpuMath.PerformanceTests/PerformanceTests.cs#L14

Should those be updated as well?

[safern]

According to our corefx coding guidelines const fields should use PascalCasing.

Note entry No.12: https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md

[jwood803]

Apologies, @safern, but just to confirm...this line protected const int IDXLEN = 1000003; would be updated to protected const int IdxLen = 1000003;, correct?

[safern]

would be updated to protected const int IdxLen = 1000003;, correct?

Correct.

cc: @eerhardt just in case ML prefers all capital. I couldn't find a doc here, so I guess we're just following corefx's guidelines?

[eerhardt]

Apologies, @safern, but just to confirm...this line protected const int IDXLEN = 1000003; would be updated to protected const int IdxLen = 1000003;, correct?

I would name it using full words, not abbreviations. Len => Length. I'm not sure what IDX stands for? Index?

[briancylui]

@eerhardt: Yes, IDX stands for Index. Sorry for the abbreviation. Inside the functions in the same class, src stands for source, dst stands for destination, and idx stands for index, but probably the other variables' names like IDXLEN could be more explicit.

[safern]

Fixed in: #943 thanks @jwood803 😄