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 examples of Set::IntSpan::Partition usage #11

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@paultcochrane
Contributor

paultcochrane commented Feb 6, 2015

The examples added in this PR are attempts at using Set::IntSpan::Partition in a scenario useful to a potential user of the module. However, I'm not 100% sure that they are good examples, since each basically expects that the intervals are merged, which S::IS::P doesn't do. Comments and recommendations for improvements most certainly welcome.

@hoehrmann

This comment has been minimized.

Owner

hoehrmann commented Feb 7, 2015

There are a couple of problems here. The Google example seems to contain content from the coordinate example at the bottom and the expected output noted in the comment is incorrect. The coordinate example seems reasonable.

@paultcochrane

This comment has been minimized.

Contributor

paultcochrane commented Feb 7, 2015

Oops! I'd left the comments from the next example I wanted to have in the original file. Sorry about that; and thanks for spotting the error! I've updated the PR by removing the extra comment lines.

Regarding the expected output: this is the expected output from the interview question mentioned in the link to the original problem (http://www.careercup.com/question?id=13014685). The output is incorrect from the point of view of Set::IntSpan::Partition, however correct if one considers that the ranges could be merged. This is what I meant in my initial comment to this PR, that I wasn't 100% sure about the examples (to a certain degree, even their relavance to S::IS::P). I can change the expected output if you wish, so that the example is consistent with S::IS::P. What do you think?

@hoehrmann

This comment has been minimized.

Owner

hoehrmann commented Feb 7, 2015

Ah I did not look at the interview question. The point of the module is basically the opposite, you start with overlapping sets and then create non-overlapping subsets of them by splitting them until each integer belongs to a distinct set. (Or if you start with non-overlapping intervals, add a new one such that the same integers are covered, but the intervals stay non-overlapping). It seems confusing to link the interview question to the module.

@paultcochrane

This comment has been minimized.

Contributor

paultcochrane commented Feb 7, 2015

In which case I think having the interview question example is superfluous; the bioinformatics question brings more relevance to the examples. Shall I submit a new PR with just the bioinformatics question, and remove the incomplete intspan.pl example?

@hoehrmann

This comment has been minimized.

Owner

hoehrmann commented Feb 7, 2015

Sounds good to me, also make sure the MANIFEST is updated accordingly, that seems to be missing from this PR.

@paultcochrane paultcochrane referenced this pull request Feb 7, 2015

Merged

Add new module example #14

@paultcochrane

This comment has been minimized.

Contributor

paultcochrane commented Feb 7, 2015

This PR replaced by PR #14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment