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

MPI testing #66

Merged
merged 15 commits into from Jul 21, 2021
Merged

MPI testing #66

merged 15 commits into from Jul 21, 2021

Conversation

claireh93
Copy link
Member

@claireh93 claireh93 commented Jun 11, 2021

Further testing to MPI to check answers are the same under different numbers of processes.
Add gather_abundance() to pull all abundances to the root node in the correct order.

@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #66 (be08ad3) into main (0fd854e) will increase coverage by 0.19%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
+ Coverage   72.47%   72.66%   +0.19%     
==========================================
  Files          26       26              
  Lines        1951     1961      +10     
==========================================
+ Hits         1414     1425      +11     
+ Misses        537      536       -1     
Impacted Files Coverage Δ
src/EcoSISTEM.jl 100.00% <ø> (ø)
src/Ecosystem.jl 75.62% <ø> (ø)
src/MPIEcosystem.jl 52.94% <88.88%> (+5.48%) ⬆️
src/MPILandscape.jl 100.00% <0.00%> (+8.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fd854e...be08ad3. Read the comment docs.

@coveralls
Copy link

coveralls commented Jun 11, 2021

Pull Request Test Coverage Report for Build 1051907321

  • 8 of 9 (88.89%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 55.207%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/MPIEcosystem.jl 8 9 88.89%
Totals Coverage Status
Change from base Build 1029465038: 0.2%
Covered Lines: 1442
Relevant Lines: 2612

💛 - Coveralls

@claireh93 claireh93 changed the title WIP: mpi MPI testing Jun 11, 2021
@claireh93
Copy link
Member Author

@richardreeve I think this is ready for you to have a look at now

Copy link
Member

@richardreeve richardreeve left a comment

Choose a reason for hiding this comment

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

Generally looks okay, and maybe we can ignore the code coverage / testing problem for now, but if you can see a way of fixing it, that would be great...

src/MPIEcosystem.jl Show resolved Hide resolved
src/MPIEcosystem.jl Outdated Show resolved Hide resolved
test/test_MPI.jl Outdated Show resolved Hide resolved
test/test_MPI.jl Show resolved Hide resolved
test/test_MPI.jl Show resolved Hide resolved
test/test_MPI.jl Outdated Show resolved Hide resolved
claireh93 and others added 4 commits June 30, 2021 16:52
Co-authored-by: Richard Reeve <git@richardreeve.net>
Co-authored-by: Richard Reeve <git@richardreeve.net>
src/MPILandscape.jl Outdated Show resolved Hide resolved
@richardreeve
Copy link
Member

@claireh93 I see your problem here. I hadn't realised this never worked before. For the time being your code works, so let's stick with it and remove the allocation later. It's not easy at all to see how to fix this. I think fundamentally we may need something like a view into an array - possibly in the root process - or distributed calculation of the diversities and abundances...

So, assuming this works I think we should just merge it as is.

Copy link
Member

@richardreeve richardreeve left a comment

Choose a reason for hiding this comment

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

Looks good to me now.

@richardreeve
Copy link
Member

richardreeve commented Jul 21, 2021

PS Another issue I noticed is that (as I'm sure you've noticed) JLD is broken and generates warnings - we need to move away from it I guess. I don't know if the solution is to use the data pipeline or to switch to JLD2, but I'll create an issue about that too. EDIT: now done - #71

@claireh93 claireh93 merged commit d9922b3 into main Jul 21, 2021
@claireh93 claireh93 deleted the claireh93/mpi branch July 21, 2021 11:13
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

3 participants