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

Implemented segmented algorithms for partitioned vector #2725

Merged
merged 45 commits into from Jul 24, 2017

Conversation

Projects
None yet
4 participants
@ghost

ghost commented Jun 29, 2017

Tasks completed as part of GSoC project (See: #1338) -

  • Moved unit tests for segmented algorithms from component to
    parallel/segmented_algorithms
  • Implemented segmented for_each_n using existing segmented for_each
  • Implemented segmented unary transform
  • Implemented 2 versions of segmented binary transform with 4 and 5
    iterators as arguments
  • Implemented segmented reduce
  • Implemented segmented transform_inclusive/exclusive_scan by
    modifying existing inclusive/exculsive_scan to accept one more
    default parameter (unary operator) as input
  • modified dispatch.hpp in both algorithms/detail and
    segmented_algorithms/detail to add helper functions for output type
    inference and casting

All algorithms have associated passing tests in the unit/parallel/segmented_algorithms folder.
Inspect shows 0 errors
This branch has been updated with the almost latest hpx/master and only one test unrelated to my code fails (68 - tests.regressions.lcos_dir.wait_for_1751), which has been fixed in later commits.

Ajai V George added some commits Jun 21, 2017

Ajai V George
Implemented segmented algorithms extending existing parallel algorithms
Tasks completed as part of GSoC project -
  - Moved unit tests for segmented algorithms from component to
    parallel/segmented_algorithms
  - Implemented segmented for_each_n using existing segmented for_each
    along with test
  - Implemented segmented unary transform along with test
  - Implemented 2 versions of segmented binary transform with 4 and 5
    iterators as parameters along with tests for both
  - Implemented segmented reduce along with test
  - Implemented segmented transform_inclusive/exclusive_scan by
    modifying existing inclusive/exculsive_scan to accept one more
    default parameter (unary operator) as input, along with tests
    both
  - modified dispatch in both algorithms/detail and
    segmented_algorithms/detail to add helper functions for output
    inference and casting
See: #1338
Ajai V George
Squashed commit of the following:
commit 1c28b70
Author: Ajai V George <ajaivgeorgedev@gmail.com>
Date:   Wed Jun 21 18:50:46 2017 +0530

    Updated tests for other algorithms for multiple localities

commit 16a3660
Author: Ajai V George <ajaivgeorgedev@gmail.com>
Date:   Wed Jun 21 18:25:42 2017 +0530

    Updated tests for for_each to run on multiple localities
Ajai V George
Updated file headers
Updated file headers with last updated date and also changed the
copyright for new files
Ajai V George
Fixed Inspect errors
Fixed 5 Inspect errors related to missing includes

@ghost ghost changed the title from Implemented segmented algorithms extending existing parallel algorithms to Implemented segmented algorithms for partitioned vector Jun 29, 2017

@hkaiser hkaiser added this to the 1.1.0 milestone Jun 29, 2017

Ajai V George
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 17, 2017

@hkaiser , @mcopik please have a look and see if this PR is ready for merging.

ghost commented Jul 17, 2017

@hkaiser , @mcopik please have a look and see if this PR is ready for merging.

@mcopik

This comment has been minimized.

Show comment
Hide comment
@mcopik

mcopik Jul 17, 2017

Contributor

@ajaivgeorge Have you fixed the issue with ctest and verified that all tests are passing on multiple localities?

Contributor

mcopik commented Jul 17, 2017

@ajaivgeorge Have you fixed the issue with ctest and verified that all tests are passing on multiple localities?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 17, 2017

@mcopik, yes (going by the CI test below). I have modified the CMakeLists file in the segmented_algorithms test folder. Please check that. I am no longer trusting the results from my laptop. Running on Rostam right now.

ghost commented Jul 17, 2017

@mcopik, yes (going by the CI test below). I have modified the CMakeLists file in the segmented_algorithms test folder. Please check that. I am no longer trusting the results from my laptop. Running on Rostam right now.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 17, 2017

@mcopik, Only, transform fails on rostam. I forgot that CI wasn't running the tests. I will fix this first and then get back to find_end/first_of. Are you sure find test fails? I ran it on rostam and there was no SegFault. Find end failed as expected.

ghost commented Jul 17, 2017

@mcopik, Only, transform fails on rostam. I forgot that CI wasn't running the tests. I will fix this first and then get back to find_end/first_of. Are you sure find test fails? I ran it on rostam and there was no SegFault. Find end failed as expected.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 19, 2017

@mcopik Errors have been fixed. Please review.

ghost commented Jul 19, 2017

@mcopik Errors have been fixed. Please review.

Ajai V George added some commits Jul 19, 2017

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jul 21, 2017

Member

Is this ready to be reviewed now?

Member

hkaiser commented Jul 21, 2017

Is this ready to be reviewed now?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 21, 2017

@hkaiser Yes it is ready to be reviewed.

ghost commented Jul 21, 2017

@hkaiser Yes it is ready to be reviewed.

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jul 21, 2017

Member

@ajaivgeorge Overall, very nice job - thanks for your effort!

Member

hkaiser commented Jul 21, 2017

@ajaivgeorge Overall, very nice job - thanks for your effort!

Ajai V George added some commits Jul 21, 2017

Ajai V George
Ajai V George
@mcopik

This comment has been minimized.

Show comment
Hide comment
@mcopik

mcopik Jul 23, 2017

Contributor

@ajaivgeorge I don't see any other problems besides those mentioned by Hartmut already. For the scan, it makes sense to verify if part_begin is not equal to end after an incrementation. Indentation in typedefs should be consistent and it's not, e.g. compare lines 818, 829 and 832 in transform.hpp; sometimes you use indentation for template parameters and sometimes not, sometimes the indentation is used in the last line and sometimes not.

@hkaiser can we enforce a CI build on AppVeyor? It seems it failed due to a network problem.

Contributor

mcopik commented Jul 23, 2017

@ajaivgeorge I don't see any other problems besides those mentioned by Hartmut already. For the scan, it makes sense to verify if part_begin is not equal to end after an incrementation. Indentation in typedefs should be consistent and it's not, e.g. compare lines 818, 829 and 832 in transform.hpp; sometimes you use indentation for template parameters and sometimes not, sometimes the indentation is used in the last line and sometimes not.

@hkaiser can we enforce a CI build on AppVeyor? It seems it failed due to a network problem.

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jul 23, 2017

Member

can we enforce a CI build on AppVeyor? It seems it failed due to a network problem.

AppVeyor seems to have problems to pull from repository forks... I can try rebuilding, though.

Member

hkaiser commented Jul 23, 2017

can we enforce a CI build on AppVeyor? It seems it failed due to a network problem.

AppVeyor seems to have problems to pull from repository forks... I can try rebuilding, though.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 23, 2017

@hkaiser, @mcopik. I have improved the indentation in segmented/transform.hpp and have added the required checks to the part_begin in the scans. Do let me know if any more changes are required.

ghost commented Jul 23, 2017

@hkaiser, @mcopik. I have improved the indentation in segmented/transform.hpp and have added the required checks to the part_begin in the scans. Do let me know if any more changes are required.

@hkaiser

LGTM, thanks!

Ajai V George
@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jul 24, 2017

Member

@mcopik: are you fine with going ahead and merging this?

Member

hkaiser commented Jul 24, 2017

@mcopik: are you fine with going ahead and merging this?

@mcopik

This comment has been minimized.

Show comment
Hide comment
@mcopik

mcopik Jul 24, 2017

Contributor

@hkaiser Yes, let's do it :)

Contributor

mcopik commented Jul 24, 2017

@hkaiser Yes, let's do it :)

@hkaiser hkaiser merged commit 0f46913 into STEllAR-GROUP:master Jul 24, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jul 25, 2017

Member

@ajaivgeorge, @mcopik: two of the new tests are failing in distributed runs, see here for an example: http://rostam.cct.lsu.edu/builders/hpx_gcc_4_9_4_boost_1_56_centos_x86_64_release/builds/126/steps/run_unit_tests/logs/stdio

Could you fix those asap, please?

Member

hkaiser commented Jul 25, 2017

@ajaivgeorge, @mcopik: two of the new tests are failing in distributed runs, see here for an example: http://rostam.cct.lsu.edu/builders/hpx_gcc_4_9_4_boost_1_56_centos_x86_64_release/builds/126/steps/run_unit_tests/logs/stdio

Could you fix those asap, please?

@aserio

This comment has been minimized.

Show comment
Hide comment
@aserio

aserio Jul 25, 2017

Contributor

@ajaivgeorge, congratulations on your commit to HPX. Once you get the Buildbot issues sorted out, contact me so that we can send you a T-Shirt

Contributor

aserio commented Jul 25, 2017

@ajaivgeorge, congratulations on your commit to HPX. Once you get the Buildbot issues sorted out, contact me so that we can send you a T-Shirt

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 26, 2017

@hkaiser @mcopik, The error has been fixed in my master branch. The issue was with the util/loop accumulate function that I added. During the PR review when i changed from using a temp iterator to using first iterator directly i forgot to add a ++first. Hadn't run the test after this change.

I have tested the algorithm upto localities=5. The other algorithms like transform_scan test and for_each and transform also work for localities > 2. But the following tests fail for localities > 2

  1. handle_values
  2. iter
  3. fill
  4. inclusive/exclusive_scan - I thought that this might be because of the changes I made but the transform_scan test works so I am not sure.

These might be failing because the tests are designed for localities=2 (especially the extensive tests for the scans) or because of something fundamental with the algorithm. I will look into this once I am done with the find algorithms.

ghost commented Jul 26, 2017

@hkaiser @mcopik, The error has been fixed in my master branch. The issue was with the util/loop accumulate function that I added. During the PR review when i changed from using a temp iterator to using first iterator directly i forgot to add a ++first. Hadn't run the test after this change.

I have tested the algorithm upto localities=5. The other algorithms like transform_scan test and for_each and transform also work for localities > 2. But the following tests fail for localities > 2

  1. handle_values
  2. iter
  3. fill
  4. inclusive/exclusive_scan - I thought that this might be because of the changes I made but the transform_scan test works so I am not sure.

These might be failing because the tests are designed for localities=2 (especially the extensive tests for the scans) or because of something fundamental with the algorithm. I will look into this once I am done with the find algorithms.

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jul 26, 2017

Member

@ajaivgeorge: please create a new PR for the algorithm fixes.

Member

hkaiser commented Jul 26, 2017

@ajaivgeorge: please create a new PR for the algorithm fixes.

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