Skip to content

CustomPipelineColumn #1091

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
Oct 1, 2018

Conversation

Ivanidzo4ka
Copy link
Contributor

Fixes #1043

@Ivanidzo4ka
Copy link
Contributor Author

@TomFinley Is this what you meant by

should be another subclass out of PipelineColumn for non-vector structural types.

@Ivanidzo4ka Ivanidzo4ka requested a review from sfilipi September 28, 2018 19:40
Copy link
Contributor

@Zruty0 Zruty0 left a comment

Choose a reason for hiding this comment

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

:shipit:

/// <summary>
/// For representing a custom <see cref="ColumnType"/>.
/// </summary>
/// <typeparam name="T"></typeparam>
Copy link
Contributor

@Zruty0 Zruty0 Sep 28, 2018

Choose a reason for hiding this comment

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

[](start = 8, length = 32)

remove empty #Resolved

@TomFinley
Copy link
Contributor

TomFinley commented Sep 28, 2018

Hi @Ivanidzo4ka. Note that you must also add this here, so that an analysis instance is actually created.

public static object MakeAnalysisInstanceCore<T>(Rec rec)

But this is interesting: your build actually passed. I guess no where do we actually create an analysis instance over types.

Let's have a test that will fail, since really we should have caught it in a test. This might be a good starting point...

ctx => ctx.LoadText(0).LoadAsImage().AsGrayscale().Resize(10, 8).ExtractPixels());

The goal is to have some sort of case where we have an analysis instance that includes this new type. I think this would do it:

var reader = TextLoader.CreateReader(env,
    ctx => ctx.LoadText(0).LoadAsImage());
var est = reader.MakeNewEstimator().Append(r => r.AsGrayscale().Resize(10, 8).ExtractPixels());
reader.Append(est);

That makes the test more verbose in what is from a user's perspective kind of pointless, but it would I think catch this problem.

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.

Thanks @Ivanidzo4ka ! If we could make the analysis instance creation work with this new type, and also perhaps have a test that would have caught the problem, then I'll be super happy with 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.

Thanks @Ivanidzo4ka !

@TomFinley TomFinley merged commit 161b450 into dotnet:master Oct 1, 2018
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 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