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

Set operations #281

Merged
merged 18 commits into from
Apr 11, 2018
Merged

Set operations #281

merged 18 commits into from
Apr 11, 2018

Conversation

d98762625
Copy link
Member

#236
Set operations are working as described in the example. Item delimiter respected in the output from all operations.

This gives an example of how an operation can be written as a class - it's got minimal difference to the interface with the rest of the app - just the bind in the module config. We can start thinking about declaring the operation class, inheriting from this and including metadata into the class somewhere.

Some things to make sure are OK with this PR:

  • Cartesian product - mismatched lengths are allowed but the pairs are just matched with undefined. For example, 1,2,3 a,b gives (1,a),(2,b),(3,undefined).
  • Cartesian product - the delimiter is not respected inside the cartesian product, but it is between the products. For example, 1-2 a-b gives (1,a)-(2,b)
  • Power set - Item delimiter is respected between items, but each new set is separated simply by \n.

Looking forward to any comments you have.

@d98762625 d98762625 changed the title Set operations #236 Set operations Mar 25, 2018
Copy link
Member

@n1474335 n1474335 left a comment

Choose a reason for hiding this comment

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

Looks good. We can add this after the ESM branch has been merged.

*
* @author d98762625 [d98762625@gmail.com]
* @copyright Crown Copyright 2018
* @license APache-2.0
Copy link
Member

Choose a reason for hiding this comment

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

Typo - should be a lowercase 'p'

import Utils from "../Utils.js";

/**
* Set operations.
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably rename this file to SetOps.js to fit with the convention (e.g. BitwiseOps.js)

@@ -4018,6 +4019,29 @@ const OperationConfig = {
inputType: "string",
outputType: "number",
args: []
},
"Set Operations": {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would make more sense to have separate operations for each function (i.e. one op for 'Union', one for 'Intersection', one for 'Set Difference'...). This is what we've done for the arithmetic ops ('Sum', 'Mean' etc.).

@@ -331,6 +331,7 @@ const Categories = [
"Extract EXIF",
"Numberwang",
"XKCD Random Number",
"Set Operations"
Copy link
Member

Choose a reason for hiding this comment

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

I'd add these operations to the 'Arithmetic/Logic' category.

* @returns {html}
*/
runSetOperation(input, args) {
const [sampleDelim, itemDelimiter, operation] = args;
Copy link
Member

Choose a reason for hiding this comment

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

Neat! I might start using this notation in other ops.

module: "Default",
description: "Performs set operations",
inputType: "string",
outputType: "html",
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this is html? It looks like your output is always just a simple string.

@d98762625 d98762625 changed the base branch from master to esm April 9, 2018 10:26
@d98762625
Copy link
Member Author

I've edited these operations to the new ESM format.

Each operation is now in it's own file.

@d98762625
Copy link
Member Author

@n1474335 Generally do you think we should .gitignore the auto-generated files? It would make these PRs cleaner.

@n1474335
Copy link
Member

n1474335 commented Apr 9, 2018

Yes, that would make sense.

@n1474335 n1474335 merged commit 955a082 into gchq:esm Apr 11, 2018
@n1474335
Copy link
Member

Awesome work, this will close #236 when we merge the ESM branch.

The 'Cartesian Product' op was incorrect, but that was entirely down to me specifying it wrong in #236. I've fixed it, as well as a few other bits and pieces.

@d98762625 d98762625 deleted the set-operations branch May 29, 2018 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants