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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

first stab at improving set_union #25

Merged

Conversation

WizardOfArc
Copy link

feedback is very welcome.

I'm thinking that instead of making the variadic case recursive, we could put each of the provided collections in a vector and do something around that... although keeping track of which collection had the minimum value in order to advance might be too much calculation 馃

Copy link
Owner

@mohitanand001 mohitanand001 left a comment

Choose a reason for hiding this comment

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

Cool. Please make the collections const in function arguments, and make the respective changes in hpp files too. For reference see function signature of min or max.

@mohitanand001
Copy link
Owner

Meanwhile, I'll look into what can be done with the varidac implementation.

@WizardOfArc
Copy link
Author

one idea for doing union of variadic parameter of sets is to:

  1. turn that into a vector of Collections (sets)
  2. for each set in vector - sort them
  3. declare a max_seen_so_far (auto?)
  4. declare a vector of the above type (not sure how)
  5. while at least one of the sets is non empty
  6. take the first element of each and add to vector
  7. sort this vector
  8. for each element of this vector check if element == to max_seen_so_far
  9. if not, add to result and update max_seen_so_far accordingly
  10. clear vector
  11. do again with next element of each vector

@WizardOfArc
Copy link
Author

@farziengineer sorry I didn't see the hpp file the first time around... I noticed that the signature in the .hpp file for max and min does not have const in it, but the .cpp file does. I'm making my change accordingly.... I also noticed that the header file does not have declarations for intersect and union of arity 2 - does that need to be in there?

@mohitanand001
Copy link
Owner

No problem 馃檪. Actually you are right, some function declaration are not there in .hpp files.#12

@mohitanand001 mohitanand001 merged commit 109fcb4 into mohitanand001:master Oct 3, 2018
@WizardOfArc WizardOfArc deleted the issue_24/more_optimal_union branch October 3, 2018 15:06
@mohitanand001
Copy link
Owner

Hi please configure your email and username locally, otherwise your contributions won't be shown.

@WizardOfArc
Copy link
Author

@farziengineer how do I do that? Set the config locally and then push again?

@mohitanand001
Copy link
Owner

I do not think this would be a good idea. You can configure it now and your future commits would be shown on the profile. You can also search on Google if there is some workaround.

@WizardOfArc
Copy link
Author

My friend helped me add my other committing email to my github account - so now it should be fine 馃樃

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

2 participants