Skip to content

[SYSTEMDS-2661, 2662]: Pipelines Optimizer and various minor built-ins#1055

Closed
Shafaq-Siddiqi wants to merge 2 commits into
apache:masterfrom
Shafaq-Siddiqi:pipelinesOptimizer
Closed

[SYSTEMDS-2661, 2662]: Pipelines Optimizer and various minor built-ins#1055
Shafaq-Siddiqi wants to merge 2 commits into
apache:masterfrom
Shafaq-Siddiqi:pipelinesOptimizer

Conversation

@Shafaq-Siddiqi
Copy link
Copy Markdown
Contributor

This commit contains,

  1. Optimizer for cleaning pipelines
  2. Minor built-ins imputeByMean, imputeByMedian, frameSort, vectorToCsv.dml
  3. minor fixes for resolving warnings in different dml scripts

This commit contains,
1. Optimizer for cleaning pipelines
2. Minor built-ins imputeByMean, imputeByMedian, frameSort, vectorToCsv.dml
3. minor fixes for resolving warnings in different dml scripts
Copy link
Copy Markdown
Contributor

@corepointer corepointer left a comment

Choose a reason for hiding this comment

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

Please fix the mentioned external dependencies. I ran the Enumerator and Bandit JUnit tests just fine after rebasing to current main branch, so this should be merge-able once the issues are addressed.
Thx for your effort @Shafaq-Siddiqi ! 😄

Comment on lines +47 to +50
public void testEnumerator(){runEnumerator(Types.ExecMode.SINGLE_NODE);};


private void runEnumerator(LopProperties.ExecType instType)
private void runEnumerator(Types.ExecMode instType)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think that change is necessary. Common practice is to name your test testEnumeratorCP and use ExecType.

{
# load the primitives
physical = as.frame("")
resource_dir = "./scripts/staging/pipelines/";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In a builtin function we shouldn't rely on external data that is not shipped in the binary distribution (the staging directory is specifically excluded when building the release artifact).

return (List[Unknown] paramList)
{
# load the hyper-parameters values
resource_dir = "./scripts/staging/pipelines/";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here - don't rely on external files that are not shipped.

@Shafaq-Siddiqi
Copy link
Copy Markdown
Contributor Author

The functionalities with minor improvements are merged into a new PR with the same name.

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.

2 participants