Skip to content

Remove ISchema in TextLoader.cs and TextLoaderCursor.cs #2140

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 3 commits into from
Jan 15, 2019

Conversation

wschin
Copy link
Member

@wschin wschin commented Jan 14, 2019

As title. This might be the tail of #1501.

@wschin wschin self-assigned this Jan 14, 2019
{
/// <summary>
/// <see cref="Infos"/>[i] stores the i-th column's name type loaded from the input text file.
Copy link
Contributor

@TomFinley TomFinley Jan 14, 2019

Choose a reason for hiding this comment

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

name type [](start = 64, length = 9)

Not a big deal perhaps since this is not a public comment, but what is a "name type"? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

name type is changed to name and type. Thanks.


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

// Empty iff either header+ not set in args, or if no header present, or upon load
// there was no header stored in the model.
/// <summary>
/// Empty iff either header+ not set in args, or if no header present, or upon load
Copy link
Contributor

@TomFinley TomFinley Jan 14, 2019

Choose a reason for hiding this comment

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

either header+ not set in args [](start = 26, length = 30)

I might prefer something like an actual reference using a <see tag to the Arguments.HasHeader field, given that we're now making this an XML comment, and we're working to de-emphasize the role of the command line in our code documentation. Not a big deal though. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Now we have

            /// <summary>
            /// Empty if <see cref="ArgumentsCore.HasHeader"/> is false, no header presents, or upon load
            /// there was no header stored in the model.
            /// </summary>

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

@TomFinley
Copy link
Contributor

        public readonly Segment[] Segments;

The reason why we had things like Name nad Kind stored in this array was to support the implementation of ISchema, which no longer exists. Yet, we are still storing it as part of this, and creating the parallel structure in the Schema itself. Should we get rid of the information that is redundant and duplicated from the schema itself, retaining all the stuff we actually still need like this segment stuff, but otherwise relying on the schema? Or is that perhaps too large of a change?


Refers to: src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs:449 in f690af1. [](commit_id = f690af1, deletion_comment = False)

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.

Looks fine @wschin , my major comment is just wondering if we can make the TextLoader.ColInfo more efficient since much of its information is now redundant, with your introduction of Schema instead of ISchema. Still a definite improvement, and what I am suggesting would not be user facing, just an efficiency/code hygiene thing.

@wschin
Copy link
Member Author

wschin commented Jan 15, 2019

@TomFinley, Thank you! Let me open an issue for cleaning TextLoader.ColInfo because some utility functions rely on that type so removing may incur a mid-scale re-factorization.

// Empty iff either header+ not set in args, or if no header present, or upon load
// there was no header stored in the model.
/// <summary>
/// Empty if <see cref="ArgumentsCore.HasHeader"/> is false, no header presents, or upon load
Copy link
Member

@sfilipi sfilipi Jan 15, 2019

Choose a reason for hiding this comment

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

false [](start = 66, length = 5)

#Resolved

Copy link
Member

@sfilipi sfilipi left a comment

Choose a reason for hiding this comment

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

:shipit:

@wschin wschin merged commit 5f9abe3 into dotnet:master Jan 15, 2019
@wschin wschin deleted the no-text-loader-ischema branch January 15, 2019 15:22
@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 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