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

New functions for checking multiple sets #1925

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

linuswagner
Copy link

Add two functions to Set library:

  1. intersection to get the intersection of multiple sets
  2. isDisjoined to check if multiple sets are pairwise disjoined

@linuswagner
Copy link
Author

Functionality doesn't have tests. Where do the tests go for the Set library?
Also, I don't like that intersection uses the getFirstFrom function to be able to use the reducer. Any idea how to better implement that?

Copy link

codecov bot commented Mar 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49%. Comparing base (39f96ec) to head (d29bdf4).
Report is 15 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##              main   #1925   +/-   ##
=======================================
  Coverage       49%     49%           
- Complexity    6161    6162    +1     
=======================================
  Files          661     661           
  Lines        58698   58701    +3     
  Branches      8547    8548    +1     
=======================================
+ Hits         28958   28961    +3     
- Misses       27549   27552    +3     
+ Partials      2191    2188    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jurgenvinju
Copy link
Member

Very nice additions and necessary to the library. Thanks @linuswagner

@DavyLandman
Copy link
Member

Hi @linuswagner,

thanks for the nice addition. Could you at at least a few tests to the lang::rascal::tests::library::Set module?

@linuswagner
Copy link
Author

@jurgenvinju I can't find the explanation of when in combination with pattern matching in the documentation, but I assume that in the case of

public bool isPairwiseDisjoint([set[&T] a, set[&T] b, *_]) = false when a & b != {};
public default bool isPairwiseDisjoint([set[&T] a, set[&T] b, *_]) = true;

it does not actually match pairs, but actually keeps a fixed, because isPairwiseDisjoint([{1,2}, {3,4}, {2}]) is true, should be false, and switching the order of the last two elements results in false again.
Or is the problem that I switched from a set to a list?

@linuswagner
Copy link
Author

Hi @linuswagner,

thanks for the nice addition. Could you at at least a few tests to the lang::rascal::tests::library::Set module?

Perfect, that's what I was looking for. Thanks!

I've decided to make the operations stricter and throw exceptions when the input has not at least 2 elements.
The decision here is that I rather have something that is supposed to compare tell me if it can't compare rather than that it masks that.
On the downside, others will always have to check before calling the function, which causes more code and could cause weird cornercases (e.g. most of the times, the program produces inputs with at least 2 sets, but rarely, it gives less than that).

Is Rascal generally "better safe than sorry" or "you should better know what you're doing"?

@DavyLandman
Copy link
Member

Hi @linuswagner,
thanks for the nice addition. Could you at at least a few tests to the lang::rascal::tests::library::Set module?

Perfect, that's what I was looking for. Thanks!

Cool 👍🏼

I've decided to make the operations stricter and throw exceptions when the input has not at least 2 elements. The decision here is that I rather have something that is supposed to compare tell me if it can't compare rather than that it masks that. On the downside, others will always have to check before calling the function, which causes more code and could cause weird cornercases (e.g. most of the times, the program produces inputs with at least 2 sets, but rarely, it gives less than that).

Is Rascal generally "better safe than sorry" or "you should better know what you're doing"?

I think better safe than sorry. We don't want someone to base their conclusion on something where rascal was just sneakily hiding an error the user made. So if there is no default correct behavior for an input with 1 element, then yeah, throw an exception.

public bool isPairwiseDisjoint(wholeInput:list[set[&T]] sets) {
int sizeSets = size(sets);
if (sizeSets == 0 || sizeSets == 1) {
throw IllegalArgument(wholeInput, "Only two or more sets can be pairwise disjoint.");
Copy link
Member

Choose a reason for hiding this comment

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

sets == wholeInput, no need to give it two names right?


test bool testIsPairwiseDisjointIdenticalElements() {return isPairwiseDisjoint([{1}, {1}]) == false;}
test bool testIsPairwiseDisjointNoOverlap() {return isPairwiseDisjoint([{1,2},{3,4},{5,6}]) == true;}
test bool testIsPairwiseDisjointOverlap() {return isPairwiseDisjoint([{1,2}, {-4,5}, {1,6,7}]) == false;}
Copy link
Member

Choose a reason for hiding this comment

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

if possible, think of an way to write an random tests. something like:

test bool testRandomPairwiseDisjoint(set[value] a, set[value] b) = (a & b == {}) == isPairwiseDisjoint([a,b]);

or even better if you could do it without using the intersection operator.

@jurgenvinju
Copy link
Member

Looking at this to merge the contributions. Currently wrestling with merge conflicts.

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.

3 participants