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

Local index addition for block distributed sparse domains #13411

Merged
merged 10 commits into from
Jul 25, 2019

Conversation

e-kayrakli
Copy link
Contributor

@e-kayrakli e-kayrakli commented Jul 10, 2019

Addresses #12673 by adding addOn argument to the bulkAdd interface.

In order to track the local domain object, this PR also adds myLocDom to SparseBlockDom class similar to myLocArr.

While implementing this, I noticed the problem with the nnz field for SparseBlock. See #13410. I think that issue must be resolved before this PR is merged. The issue is somewhat subtle and hard to bump into. However, local index addition implemented in this PR will mess up nnz and probably none of the locales will be able to read the correct size of the domain.

TODO:

  • Added test passes with CHPL_COMM=gasnet
  • test/users/engin/sparse_bulk* passes
  • Full paratest (local and gasnet)
  • Doc
  • Check if reasonable errors are generated when indices are OOB

@e-kayrakli e-kayrakli changed the title Add bulkAddHere for block distributed sparse domains Local index addition for block distributed sparse domains Jul 22, 2019
@e-kayrakli
Copy link
Contributor Author

@benharsh -- do you mind giving this a look?

Copy link
Member

@benharsh benharsh left a comment

Choose a reason for hiding this comment

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

This generally looks fine to me. I'd prefer that we have a better default value for locales than nil, but that's OK for now.

@e-kayrakli
Copy link
Contributor Author

Created #13526 to capture the special locale value idea.

Running some more tests to make sure I haven't missed anything. Thanks @benharsh

@e-kayrakli
Copy link
Contributor Author

The word "nil" for the default argument value is dropped in docs for some reason, filed an issue: #13531

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