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

make origin and target write size match in the mpi_put call #1117

Merged
merged 3 commits into from
Aug 20, 2021

Conversation

clevelam
Copy link
Collaborator

@clevelam clevelam commented Aug 20, 2021

Background

  • This fixes the MPI_Put call in quick_index to specify the target buffer size equal to the origin buffer size. Originally @clevelam thought that it was supposed to be the off processor buffer size. This might also explain why we had some issues at large mpi_put writes (over 10K). For now we will leave in the chunk writing to save temporary memory space.

Purpose of Pull Request

Description of changes

  • Remove un-needed off processor buffer size
  • Make target buffer size equal to the origin buffer size in the MPI_Put call used by quick_index

Status

@clevelam clevelam requested a review from KineticTheory August 20, 2021 15:00
@clevelam clevelam self-assigned this Aug 20, 2021
@clevelam clevelam added the bug label Aug 20, 2021
@clevelam clevelam added this to the Draco-7_12_0 milestone Aug 20, 2021
@KineticTheory
Copy link
Collaborator

@alexrlongne Can you review this change?

@KineticTheory
Copy link
Collaborator

I'm curios to see if this change will fix the test failure on MSVC. I'll try it now.

@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #1117 (a6a6456) into develop (fc46b6d) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           develop   #1117     +/-   ##
=========================================
- Coverage     89.0%   88.9%   -0.1%     
=========================================
  Files          373     373             
  Lines        19573   19568      -5     
=========================================
- Hits         17420   17415      -5     
  Misses        2153    2153             

@alexrlongne
Copy link
Contributor

LGTM

@alexrlongne
Copy link
Contributor

I don't understand the drop in coverage so I'm good to merge whenever.

@KineticTheory
Copy link
Collaborator

KineticTheory commented Aug 20, 2021

I don't understand the drop in coverage so I'm good to merge whenever.

I don't like the way CodeCov does this. The PR has 5 less lines of code and so it has 5 fewer lines 'hit' by the coverage checker. This equates to a loss of coverage.

@KineticTheory
Copy link
Collaborator

I'm curios to see if this change will fix the test failure on MSVC. I'll try it now.

It still fails. We'll omit the 3 rank tests when using MSVC.

@KineticTheory KineticTheory merged commit d81b6e6 into lanl:develop Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants