-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
switch housing dataset to wine #170
switch housing dataset to wine #170
Conversation
address #3 |
build.proj
Outdated
</TestFile> | ||
</ItemGroup> | ||
|
||
<Target Name="DownloadExternalTestFiles" Inputs="@(TestFile)" Outputs="%(TestFile.Result)"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do me a favor and test this with 2 or 3 datasets downloaded from the internet? (I guess it can be any file, it doesn't have to be a .csv file) I just want to ensure this works with more than 1 file, and it works correctly when doing it a 2nd time that the data sets aren't downloaded on subsequent builds.
That way the next person who needs to add a dataset isn't bit by a bug here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<ItemGroup> <TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/winequality-white.csv" Url="https://archive.ics.uci.edu/ml/machine-learning-databases/wine-quality/winequality-white.csv"> <DestinationFile>$(MSBuildThisFileDirectory)test/data/external/winequality-white.csv</DestinationFile> </TestFile> <TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/image.png" Url="https://www.google.com/logos/doodles/2018/tamara-de-lempickas-120th-birthday-4614326680813568-l.png"> <DestinationFile>$(MSBuildThisFileDirectory)test/data/external/image.png</DestinationFile> </TestFile> <TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/logo.png" Url="https://www.baidu.com/img/bd_logo1.png"> <DestinationFile>$(MSBuildThisFileDirectory)test/data/external/logo.png</DestinationFile> </TestFile> </ItemGroup>
test it with this. tried different combinations, they all work
Is there are any way to format it properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! nice to hear it works for many items.
As for the formatting, if you use triple back-ticks ``` at the start of a line, markdown will show exactly what you type until the next set of triple back-ticks. This is useful for displaying code. You can even put a code hint after the triple back-ticks XML, and markdown will syntax highlight. So if I take your comment above it looks like this:
<ItemGroup>
<TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/winequality-white.csv" Url="https://archive.ics.uci.edu/ml/machine-learning-databases/wine-quality/winequality-white.csv">
<DestinationFile>$(MSBuildThisFileDirectory)test/data/external/winequality-white.csv</DestinationFile>
</TestFile>
<TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/image.png" Url="https://www.google.com/logos/doodles/2018/tamara-de-lempickas-120th-birthday-4614326680813568-l.png">
<DestinationFile>$(MSBuildThisFileDirectory)test/data/external/image.png</DestinationFile>
</TestFile>
<TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/logo.png" Url="https://www.baidu.com/img/bd_logo1.png"> <DestinationFile>$(MSBuildThisFileDirectory)test/data/external/logo.png</DestinationFile>
</TestFile>
</ItemGroup>
See https://guides.github.com/features/mastering-markdown/#GitHub-flavored-markdown for more info.
I see a few more references to
Can you update them all? |
build.proj
Outdated
|
||
|
||
<ItemGroup> | ||
<TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/winequality-white.csv" Url="https://archive.ics.uci.edu/ml/machine-learning-databases/wine-quality/winequality-white.csv"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, I believe the attribute and subelement formats are equivalent, you might want to pick one or the other. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't move Include to subelement, but I can move Url to it. having everything as attribute is looks too long
In reply to: 189065564 [](ancestors = 189065564)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a requesting change here, just giving some information for future reference.
The Include
attribute on an MSBuild Item is "special". It has to be an XML attribute. Before MSBuild v15, all "custom" metadata (like your Url attribute) could NOT be put as an XML attribute - instead it had to be a sub-element. In v15, they changed it so custom metadata now can be in either an attribute or sub-element.
A format I like and use regularly is the following:
<TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/winequality-white.csv"
Url="https://archive.ics.uci.edu/ml/machine-learning-databases/wine-quality/winequality-white.csv"
DestinationFile="$(MSBuildThisFileDirectory)test/data/external/winequality-white.csv" />
I think it is compact, reads easy, and is easy to maintain/update.
@dotnet-bot test Windows_NT Debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyone other than Eric can look on this? @glebuk |
public void EntryPointEvaluateRegression() | ||
{ | ||
var dataPath = GetDataPath("housing.txt"); | ||
var dataPath = GetDataPath(@"external/winequality-white.csv"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@"external/winequality-white.csv [](start = 39, length = 32)
Extract dataset info into static dataset classes so that no need to repeat paths and schemas. Ideally you can even just return a new loader for each dataset. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* replace housing uci dataset to wine quality
This commit replaces housing dataset to wine dataset which we download during build from external source