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

Fixes incorrect team in Pattern/Array by calling the wrong ctor unintended #354

Merged
merged 10 commits into from
Apr 12, 2017

Conversation

ddiefenthaler
Copy link
Contributor

This pull request reverts the changes from PR #304 and solves it in a different way.

With the removal of the constructors from BlockPattern it got incompatible to the CSRPattern,
as the constructor of the CSRPattern differs in the signature.

To fix the ambiguity in the constructors this pull request removes some of the default paramters.
See also TilePattern for example, it is done there in the same way. Parameter of TilePattern ctor

Due to the missing constructor the wrong constructor got called from dash::Array which lead to always using dash::Team::All() as the team of the array. This behaviour is observed in issue #353.

Fixes #353 and a part #292 (ThreadsafetyTest.ConcurrentAlgorithm).

@ddiefenthaler
Copy link
Contributor Author

@devreal Can you add your adjusted unit test from issue #353?
Thanks for that by the way, it made debugging it quite a bit simpler
than having to jump between multiple threads to figure out where the team went wrong.

Copy link
Member

@fmoessbauer fmoessbauer left a comment

Choose a reason for hiding this comment

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

If that is everything that is necessary to fix the ambiguity this PR is great! Can you please cross-check if this still works for the Coarray. Nevertheless @fuchsto or @devreal should have a look at the changes too.

@@ -249,11 +249,10 @@ class BlockPattern
/// Pattern size (extent, number of elements) in every dimension
const SizeSpec_t & sizespec,
/// Distribution type (BLOCKED, CYCLIC, BLOCKCYCLIC, TILE or NONE) of
/// all dimensions. Defaults to BLOCKED in first, and NONE in higher
/// dimensions
Copy link
Member

Choose a reason for hiding this comment

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

fix documentation (currently ends with "of")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats actually just a tricky way of git displaying it.
I fell for it too yesterday and repeated that commit.

/// all dimensions. Defaults to BLOCKED in first, and NONE in higher
/// dimensions
const DistributionSpec_t & dist = DistributionSpec_t(),
/// all dimensions.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fmoessbauer Look here, there is the rest of the line.

@devreal
Copy link
Member

devreal commented Apr 1, 2017

@ddiefenthaler Great job, thanks a lot for the quick fix! I added the check to the TeamSplit test case and it succeeds. Since it's not my code, let's wait for the review from @fuchsto.

While we are at it: Skimming through the code yesterday, I noticed that the _arguments in BlockPattern are stored in the pattern object even though they are only used in the c'tor. How about creating them temporarily and forwarding them to a private constructor? After all, they are rather heavy-weight. Maybe we could also apply some constexpr magic there because it's essentially just forwarding arguments (haven't tried though).

@fmoessbauer
Copy link
Member

fmoessbauer commented Apr 1, 2017

While we are at it: I fixed some other bugs in the CSR pattern as well. If you like, pull in (cherry-pick) 0609a02fd58e7. Btw: we definitely need concept checking on the patterns.

@ddiefenthaler
Copy link
Contributor Author

I temporarily merged with feat-coarray, it was nearly automatic possible and test ran without problems.

If desired the constructor readded in this PR could get shortend by delegating to the other constructor
(like some constructors from Array.h).

While being at cherry picking at CSRPattern I'll also add an constructor with the signatur
CSRPattern(SizeSpec, DistSpec, TeamSpec, Team) so that it is more compatible to the other patterns.

@ddiefenthaler
Copy link
Contributor Author

ddiefenthaler commented Apr 1, 2017

Delegating the constructor would look like this:

  BlockPattern(
    /// Pattern size (extent, number of elements) in every dimension
    const SizeSpec_t         & sizespec,
    /// Distribution type (BLOCKED, CYCLIC, BLOCKCYCLIC, TILE or NONE) of
    /// all dimensions. Defaults to BLOCKED in first, and NONE in higher
    /// dimensions
    const DistributionSpec_t & dist = DistributionSpec_t(),
    /// Team containing units to which this pattern maps its elements
    Team                     & team = dash::Team::All())
  : BlockPattern(sizespec, dist, TeamSpec_t(_distspec, *_team), team)
  {
    DASH_LOG_TRACE("BlockPattern()", "(sizespec, dist, team)");
    DASH_LOG_TRACE("BlockPattern()", "finished delegation constructor");
  }

against the current implementation.

@devreal, @fuchsto is this preferred? This would be applicable to most patterns.

Otherwise this pull request would be ready to merge in my opinion.
EDIT: Sorry @devreal missed your comment on the second reading...
Of course it is not a bad idea to get rid of the _arguments member as it should be only needed during the construction.

@ddiefenthaler
Copy link
Contributor Author

@fuchsto Here is a patch which would include the delegating constructors:
Changed-ctor-of-pattern-to-delegate-to-similar-ctor.patch

Apply it if you like,
Otherwise this pull request should be ready for review & merge

@fuchsto fuchsto merged commit 2cbaf77 into development Apr 12, 2017
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.

dash::Array storing the wrong team?
4 participants