Skip to content
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

Add pipeline cookbook #4323

Merged
merged 7 commits into from Jun 28, 2018

Conversation

vinx13
Copy link
Member

@vinx13 vinx13 commented Jun 2, 2018

  • add pipeline meta example and cookbook
  • add transformer factory

Pipeline
========

Pipeline is a machine that chains multiple transformers and machines. It consists of a sequence of transformers as intermediate stages and a machine as the final stage. Features are transformed by transformers and fed into the next stage sequentially.
Copy link
Member

Choose a reason for hiding this comment

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

maybe add sgclass to link to the API docs

Pipeline
========

Pipeline is a machine that chains multiple transformers and machines. It consists of a sequence of transformers as intermediate stages and a machine as the final stage. Features are transformed by transformers and fed into the next stage sequentially.
Copy link
Member

Choose a reason for hiding this comment

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

could you start every sentence in a new line? I.e. newline after each ".", this makes subsequent diffs way easier to review

Pipeline
========

Pipeline is a machine that chains multiple transformers and machines. It consists of a sequence of transformers as intermediate stages and a machine as the final stage. Features are transformed by transformers and fed into the next stage sequentially.
Copy link
Member

Choose a reason for hiding this comment

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

"stage" of what? Maybe add "stage of training"


.. toctree::
:maxdepth: 1
:glob:
Copy link
Member

Choose a reason for hiding this comment

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

+1


# additional integration variables
#![extract_centers_and_radius]
RealMatrix c = kmeans.get_cluster_centers()
Copy link
Member

Choose a reason for hiding this comment

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

can this be done with the generic get?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, this is not a parameter :(

Copy link
Member

Choose a reason for hiding this comment

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

I see.
Ok, one of the next things I want to do is to register callback functions with get so that we can lazily evaluate things, like in this case

REQUIRE(
m_stages.empty() ||
holds_alternative<CTransformer*>(m_stages.back()),
"Transformers can not be placed after machines.\n");
Copy link
Member

Choose a reason for hiding this comment

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

Could you maybe make this "Last element in pipeline is %s", and then put the machine name there?

REQUIRE(
m_stages.empty() ||
holds_alternative<CTransformer*>(m_stages.back()),
"Multiple machines are added to pipeline.\n");
Copy link
Member

Choose a reason for hiding this comment

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

same story, pls print the name of the last added machine so the user knows what she did wrong

{
REQUIRE(
!m_stages.empty() && holds_alternative<CMachine*>(m_stages.back()),
"Machine has not been added.\n");
Copy link
Member

Choose a reason for hiding this comment

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

Could you be more elaborate here as well
"Pipline cannot be trained without an added machine. Last element is %s"

CLabels* CPipeline::apply(CFeatures* data)
{
REQUIRE(
!m_stages.empty() && holds_alternative<CMachine*>(m_stages.back()),
Copy link
Member

Choose a reason for hiding this comment

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

as above. user facing error messages need to be more explicit

{
REQUIRE(
!m_stages.empty() && index < m_stages.size() - 1,
"Index out of bound. There are only %d transformers.\n",
Copy link
Member

Choose a reason for hiding this comment

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

print the index as well
"Requested index (%d) out of bounds " ...


namespace shogun
{
class CPipeline : public CMachine
Copy link
Member

Choose a reason for hiding this comment

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

Could you write some minimal class docs?


EXPECT_EQ(pipeline->get_transformer(1), transformer2.get());
EXPECT_EQ(pipeline->get_machine(), machine.get());
}
Copy link
Member

Choose a reason for hiding this comment

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

great set of tests!

Copy link
Member

@karlnapf karlnapf left a comment

Choose a reason for hiding this comment

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

Great work!
I made some comments on user facing parts of this

@vigsterkr vigsterkr force-pushed the feature/transformers branch 2 times, most recently from b7d2ff9 to f211b65 Compare June 8, 2018 14:15

#![create_pipeline]
Pipeline pipeline()
pipeline.with(subMean)
Copy link
Member

Choose a reason for hiding this comment

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

this needs fixing, because of reserved keyword

#![create_pipeline]
Pipeline pipeline()
pipeline.with(subMean)
pipeline.with(pca)
Copy link
Member

Choose a reason for hiding this comment

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

change it

@vigsterkr
Copy link
Member

@vinx13

pipeline.cs(31,1): error CS0246: The type or namespace name `PipelineBuilder' could not be found. Are you missing an assembly reference?
pipeline.cs(32,1): error CS0841: A local variable `builder' cannot be used before it is declared
pipeline.cs(33,1): error CS0841: A local variable `builder' cannot be used before it is declared
pipeline.cs(34,1): error CS0246: The type or namespace name `Pipeline' could not be found. Are you missing an assembly reference?
pipeline.cs(38,1): error CS0841: A local variable `pipeline' cannot be used before it is declared
pipeline.cs(39,25): error CS0841: A local variable `pipeline' cannot be used before it is declared
Compilation failed: 6 error(s), 0 warnings```

@vigsterkr
Copy link
Member

and in java

[ 97%] Generating pipeline/pipeline.class
/opt/shogun/build/examples/meta/java/pipeline/pipeline.java:12: error: cannot find symbol
import org.shogun.Pipeline;
                 ^
  symbol:   class Pipeline
  location: package org.shogun
/opt/shogun/build/examples/meta/java/pipeline/pipeline.java:13: error: cannot find symbol
import org.shogun.PipelineBuilder;
                 ^
  symbol:   class PipelineBuilder
  location: package org.shogun
/opt/shogun/build/examples/meta/java/pipeline/pipeline.java:51: error: cannot find symbol
PipelineBuilder builder = new PipelineBuilder();
^
  symbol:   class PipelineBuilder
  location: class pipeline
/opt/shogun/build/examples/meta/java/pipeline/pipeline.java:51: error: cannot find symbol
PipelineBuilder builder = new PipelineBuilder();
                              ^
  symbol:   class PipelineBuilder
  location: class pipeline
/opt/shogun/build/examples/meta/java/pipeline/pipeline.java:54: error: cannot find symbol
Pipeline pipeline = builder.then(kmeans);
^
  symbol:   class Pipeline
  location: class pipeline
5 errors
examples/meta/java/CMakeFiles/java-pipeline-pipeline.dir/build.make:62: recipe for target 'examples/meta/java/pipeline/pipeline.class' failed
make[2]: *** [examples/meta/java/pipeline/pipeline.class] Error 1
CMakeFiles/Makefile2:5049: recipe for target 'examples/meta/java/CMakeFiles/java-pipeline-pipeline.dir/all' failed
make[1]: *** [examples/meta/java/CMakeFiles/java-pipeline-pipeline.dir/all] Error 2
Makefile:160: recipe for target 'all' failed
make: *** [all] Error 2```

@vigsterkr vigsterkr merged commit 550639e into shogun-toolbox:feature/transformers Jun 28, 2018
vigsterkr pushed a commit that referenced this pull request Jun 28, 2018
* Add transformer factory
* Add pipeline cookbook
* Fix swig include
vigsterkr pushed a commit that referenced this pull request Jul 10, 2018
* Add transformer factory
* Add pipeline cookbook
* Fix swig include
vigsterkr pushed a commit that referenced this pull request Jul 10, 2018
* Add transformer factory
* Add pipeline cookbook
* Fix swig include
vigsterkr pushed a commit that referenced this pull request Jul 12, 2018
* Add transformer factory
* Add pipeline cookbook
* Fix swig include
ktiefe pushed a commit to ktiefe/shogun that referenced this pull request Jul 30, 2019
* Add transformer factory
* Add pipeline cookbook
* Fix swig include
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants