Skip to content

updating namespaces #2914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Mar 13, 2019
Merged

updating namespaces #2914

merged 11 commits into from
Mar 13, 2019

Conversation

sfilipi
Copy link
Member

@sfilipi sfilipi commented Mar 11, 2019

Towards #2751

Microsoft.ML.LightGBM -> changes to Microsoft.ML.Trainers.LightGBM
Microsoft.ML.Transforms.FeatureSelection -> moves to Microsoft.ML.Transforms

@sfilipi sfilipi self-assigned this Mar 11, 2019
@sfilipi sfilipi added this to the 0319 milestone Mar 11, 2019
@@ -8,7 +8,7 @@
using Microsoft.ML.EntryPoints;
using Microsoft.ML.Runtime;

namespace Microsoft.ML.LightGBM
namespace Microsoft.ML.Trainers.LightGBM
Copy link
Member

@eerhardt eerhardt Mar 11, 2019

Choose a reason for hiding this comment

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

If we are changing all of these places, should we start using the new casing we decided on in #2762.

namespace Microsoft.ML.Trainers.LightGbm #Resolved

Copy link
Member Author

@sfilipi sfilipi Mar 11, 2019

Choose a reason for hiding this comment

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

How about assembly names? #Resolved

Copy link
Member

@eerhardt eerhardt Mar 12, 2019

Choose a reason for hiding this comment

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

It kind of feels like if that is our casing... we should probably case it the same way everywhere. #Resolved

@sfilipi
Copy link
Member Author

sfilipi commented Mar 12, 2019

I have renamed the package definition files in disc, but git didn't pick up the change. Will update and re-post. #Resolved

@eerhardt
Copy link
Member

eerhardt commented Mar 12, 2019

I have renamed the package definition files in disc, but git didn't pick up the change.

My suggestion would be to either use a case sensitive file system (like a Linux box), or use git mv -f on Windows. #Resolved

sfilipi added 3 commits March 12, 2019 12:39
Microsoft.ML.Transforms.FeatureSelection -> moves to Microsoft.ML.Transforms
and the ImageLoader transform moved to Microsoft.ML.Data together with the TextLoader.
@sfilipi
Copy link
Member Author

sfilipi commented Mar 12, 2019

thanks Eric.


In reply to: 472009849 [](ancestors = 472009849)

@@ -13,10 +13,10 @@
<ProjectReference Include="..\Microsoft.ML.Data\Microsoft.ML.Data.csproj" />
<ProjectReference Include="..\Microsoft.ML.Ensemble\Microsoft.ML.Ensemble.csproj" />
<ProjectReference Include="..\Microsoft.ML.FastTree\Microsoft.ML.FastTree.csproj" />
<ProjectReference Include="..\Microsoft.ML.LightGBM\Microsoft.ML.LightGbm.csproj" />
Copy link
Member

@eerhardt eerhardt Mar 12, 2019

Choose a reason for hiding this comment

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

LightGBM => LightGbm #Resolved

@eerhardt
Copy link
Member

eerhardt commented Mar 12, 2019

[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.LightGBM.StaticPipe" + PublicKey.Value)]

LightGBM => LightGbm #Resolved


Refers to: src/Microsoft.ML.LightGBM/Properties/AssemblyInfo.cs:9 in 3d181b9. [](commit_id = 3d181b9, deletion_comment = False)

@eerhardt
Copy link
Member

eerhardt commented Mar 12, 2019

<IncludeInPackage>Microsoft.ML.LightGBM</IncludeInPackage>

LightGBM => LightGbm #Resolved


Refers to: src/Microsoft.ML.LightGBM/Microsoft.ML.LightGBM.csproj:5 in 3d181b9. [](commit_id = 3d181b9, deletion_comment = False)

@@ -6,7 +6,7 @@

<ItemGroup>
<ProjectReference Include="..\Microsoft.ML.Data\Microsoft.ML.Data.csproj" />
<ProjectReference Include="..\Microsoft.ML.LightGBM\Microsoft.ML.LightGBM.csproj" />
<ProjectReference Include="..\Microsoft.ML.LightGBM\Microsoft.ML.LightGbm.csproj" />
Copy link
Member

@eerhardt eerhardt Mar 12, 2019

Choose a reason for hiding this comment

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

You changed the casing of the folder, right? \Microsoft.ML.LightGBM\ => \Microsoft.ML.LightGbm\ or else the Linux build is gonna fail. #Resolved

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Mar 12, 2019

    // This example requires installation of additional nuget package <a href="https://www.nuget.org/packages/Microsoft.ML.LightGBM/">Microsoft.ML.LightGBM</a>.

It should become LightGbm? #Resolved


Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/BinaryClassification/LightGbmWithOptions.cs:8 in cfcc7d8. [](commit_id = cfcc7d8, deletion_comment = False)

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:

@codecov
Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #2914 into master will increase coverage by <.01%.
The diff coverage is 96.96%.

@@            Coverage Diff             @@
##           master    #2914      +/-   ##
==========================================
+ Coverage   72.19%   72.19%   +<.01%     
==========================================
  Files         796      796              
  Lines      142023   142023              
  Branches    16046    16046              
==========================================
+ Hits       102527   102531       +4     
+ Misses      35116    35113       -3     
+ Partials     4380     4379       -1
Flag Coverage Δ
#Debug 72.19% <96.96%> (ø) ⬆️
#production 67.98% <0%> (ø) ⬆️
#test 88.3% <100%> (ø) ⬆️
Impacted Files Coverage Δ
...rc/Microsoft.ML.LightGbm/Parallel/SingleTrainer.cs 36.84% <ø> (ø)
src/Microsoft.ML.LightGbm/LightGbmArguments.cs 89.63% <ø> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs 63.8% <ø> (ø) ⬆️
....ML.Data/Scorers/FeatureContributionCalculation.cs 94.24% <ø> (ø) ⬆️
src/Microsoft.ML.ImageAnalytics/ImageLoader.cs 84.55% <ø> (ø) ⬆️
src/Microsoft.ML.ImageAnalytics/ImageResizer.cs 84.64% <ø> (ø) ⬆️
src/Microsoft.ML.LightGbm/LightGbmCatalog.cs 100% <ø> (ø)
...rc/Microsoft.ML.LightGbm/LightGbmRankingTrainer.cs 73.46% <ø> (ø)
...rc/Microsoft.ML.LightGbm/WrappedLightGbmDataset.cs 76.33% <ø> (ø)
...Microsoft.ML.Transforms/FeatureSelectionCatalog.cs 100% <ø> (ø) ⬆️
... and 36 more

/// <summary>
/// Support for feature contribution calculation.
/// </summary>
public sealed class FeatureContributionCalculator
Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing how this class is used anywhere in the code.

Can it be deleted?

It appears the way this is working is that we have a public API that takes in a ICalculateFeatureContribution (the public interface above) object, and then we internally cast that same object to IFeatureContributionMapper (the internal interface above that). I don't see where we are using this public class anywhere in our code.

cc @TomFinley @rogancarr

Copy link
Member Author

Choose a reason for hiding this comment

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

tried to remove it, and just use the IFeatureContributionMapper, but it will need the GetFeatureContributionMapper, and the ValueMapper to be public too. The ValueMapper is internal atm.


In reply to: 265147046 [](ancestors = 265147046)

Copy link
Member

@eerhardt eerhardt Mar 13, 2019

Choose a reason for hiding this comment

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

I was able to remove it, and make the solution build successfully:

eerhardt@cd1ac9f

(Obviously this is outside of the scope of this current PR - which is just moving namespaces. Maybe we should open an issue for this design, as I think it kind of "smells".)

Copy link
Member Author

Choose a reason for hiding this comment

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

oh ic, you've removed it completely. I was merging it with the IFeatureContributionMapper. I'll create an issue.

@eerhardt
Copy link
Member

eerhardt commented Mar 13, 2019

It doesn't look like this file has been renamed for the right casing - the file is still Microsoft.ML.LightGBM.csproj #Resolved


Refers to: src/Microsoft.ML.LightGBM/Microsoft.ML.LightGBM.csproj:1 in 47d01a7. [](commit_id = 47d01a7, deletion_comment = False)

@sfilipi sfilipi merged commit c5aab77 into dotnet:master Mar 13, 2019
@sfilipi sfilipi deleted the namespace2751 branch March 13, 2019 18:37
@ghost ghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants