EOSPAC vector implementations #177

rbberger · 2022-09-01T18:49:11Z

PR Summary

PR Checklist

Adds a test for any bugs fixed. Adds tests for new features.
Format your changes by using the make format command after configuring with cmake.
Document any new features, update documentation for changes made.
Make sure the copyright notice on any files you modified is up to date.
After creating a pull request, note it in the CHANGELOG.md file

jhp-lanl

I think this looks really good! I don't think the underlying math and thermodynamics you copied over from the existing code has been thoroughly vetted, but that's beyond the scope of this MR. There may be additional performance improvements we can add, but those can be done later.

The only changes I would ask for would be a simple function to provide a required scratch size and a simple test. There are multiple scratch sizes needed though so this requires some discussion I think. For the test, I would just pick one of these methods and make a test for the chunk version and the full array version and make sure the result agrees with a for loop over scalar lookups. We can expand the test later as part of vetting the underlying logic and math.

singularity-eos/eos/eos_eospac.hpp

Yurlungur

This is a draft, so not approving or requesting changes. A little bit of feedback below.

singularity-eos/eos/eos_eospac.hpp

Yurlungur · 2022-09-01T23:21:16Z

Also thanks for putting in the work to prototype this! I like both the "scatch" and "chunk" approaches. But I think we should be a little thoughtful of GPU performance in both cases. I don't think any of this code will run on GPU as-is.

rbberger · 2022-10-10T21:29:37Z

@Yurlungur @jhp-lanl I've updated all scratch and chunk versions to use EOSPAC options to minimize copies and do unit conversions. I've also expanded the profiling to include more methods and made it part of CTest. Below are timings I've generated per method on skylake-platinum nodes on Darwin:

jhp-lanl · 2022-10-11T19:16:50Z

Great work @rbberger ! I'm excited especially that this work is part of CTest so that we can have a baseline and look for ways to improve. We should discuss this more at the meeting tomorrow!

rbberger · 2022-10-11T20:05:58Z

@jhp-lanl yeah, having it part of CTest already pays off. I'm having weird failures on power9 that need further investigation. 😄

Yurlungur · 2022-10-11T20:14:49Z

Nice work @rbberger ! I second @jhp-lanl 's comments.

Yurlungur · 2022-10-12T20:45:52Z

Peanut gallery says we go to scratch and drop lambdas and chunks

rbberger · 2022-10-19T23:24:38Z

@Yurlungur @jhp-lanl Issue #187 blocks me from doing any meaningful GPU tests on the CI.

jhp-lanl · 2022-10-19T23:33:50Z

@Yurlungur @jhp-lanl Issue #187 blocks me from doing any meaningful GPU tests on the CI.

I seem to recall that the accessible EOSPAC spackage isn't up-to-date and I'm not quite sure what the best way to get 6.5 is. I'll ping Daniel Sheppard.

rbberger · 2022-10-21T22:23:55Z

@Yurlungur ready for review.

jhp-lanl

I'm nominally approving this because most of my comments are questions rather than critiques and I can be convinced out of any code changes.

@rbberger feel free to resolve any of my questions as you answer them or make code changes to address them.

eospac-wrapper/eospac_wrapper.cpp

python/module.cpp

python/module.hpp

singularity-eos/eos/eos_base.hpp

singularity-eos/eos/eos_eospac.hpp

test/CMakeLists.txt

test/profile_eospac.cpp

test/CMakeLists.txt

singularity-eos/eos/eos_eospac.hpp

Yurlungur

Sorry I left this so long @rbberger it slipped my mind. Overall this looks very good. A few nitpicks below.

eospac-wrapper/eospac_wrapper.cpp

singularity-eos/eos/eos_base.hpp

singularity-eos/eos/eos_eospac.hpp

test/CMakeLists.txt

Once we use C++17 this could be replaced with [[maybe_unused]]

…ized

rbberger · 2023-01-11T23:13:36Z

@Yurlungur @jhp-lanl except the point about the modifiers and GPUs, this PR should be done. It also passes on re-git. Let me know if the documentation is sufficient.

jhp-lanl

This looks really good @rbberger ! Thanks for all the effort on this!

jhp-lanl · 2023-01-12T00:04:18Z

I'll wait for the Darwin tests to pass before we merge.

Yurlungur

Looks good. 👍 Let's merge as soon as Darwin tests pass

jhp-lanl · 2023-02-16T22:54:14Z

@rbberger is there anything holding this up? To me it looks like you can merge in master (and resolve conflicts) and we can merge this.

rbberger · 2023-02-17T17:20:17Z

@rbberger is there anything holding this up? To me it looks like you can merge in master (and resolve conflicts) and we can merge this.

No. It kind of got stuck because we were looking at why GPU builds being so slow (in general, not just this), but I'll fix this one up again so we can merge it.

jhp-lanl · 2023-02-17T18:16:44Z

@rbberger is there anything holding this up? To me it looks like you can merge in master (and resolve conflicts) and we can merge this.

No. It kind of got stuck because we were looking at why GPU builds being so slow (in general, not just this), but I'll fix this one up again so we can merge it.

Let us know if you need any support getting this pushed 👍

rbberger · 2023-02-22T23:16:40Z

@Yurlungur re-git pipeline successful. feel free to merge.

Yurlungur · 2023-02-23T00:15:22Z

Thanks for all your hard work on this @rbberger ! At some point we should check to see what is necessary to pull this through for GPU implementations and modifiers. But for now I think we can celebrate speeding up the EOSPAC calls.

jhp-lanl · 2023-02-23T02:31:04Z

Yes indeed! Thanks for all the hard work @rbberger !

jhp-lanl requested changes Sep 1, 2022

View reviewed changes

singularity-eos/eos/eos_eospac.hpp Outdated Show resolved Hide resolved

singularity-eos/eos/eos_eospac.hpp Outdated Show resolved Hide resolved

singularity-eos/eos/eos_eospac.hpp Outdated Show resolved Hide resolved

singularity-eos/eos/eos_eospac.hpp Outdated Show resolved Hide resolved

Yurlungur reviewed Sep 1, 2022

View reviewed changes

rbberger force-pushed the rberger_eospac_vectorized branch 2 times, most recently from b9afe6f to d4eef73 Compare September 7, 2022 17:21

rbberger force-pushed the rberger_eospac_vectorized branch from ac4e0b8 to b67321c Compare September 24, 2022 00:44

rbberger force-pushed the rberger_eospac_vectorized branch 2 times, most recently from 6b54d26 to 644a3e0 Compare October 11, 2022 00:16

rbberger force-pushed the rberger_eospac_vectorized branch 2 times, most recently from a8a922d to 159f465 Compare October 19, 2022 23:27

rbberger added 2 commits October 20, 2022 21:38

Implement EOSPAC vector functions

da19035

Add profile_eospac test

7a02f4d

rbberger force-pushed the rberger_eospac_vectorized branch from 159f465 to 0f5fde3 Compare October 21, 2022 03:39

Add EOSPAC vectorized functions to Python bindings

8f82ef6

rbberger force-pushed the rberger_eospac_vectorized branch from 0f5fde3 to 8f82ef6 Compare October 21, 2022 04:02

rbberger changed the title ~~Draft: EOSPAC vector implementations~~ EOSPAC vector implementations Oct 21, 2022

Skip EOSPAC profiling test on GPU

3199710

jhp-lanl approved these changes Dec 1, 2022

View reviewed changes

Merge branch 'main' into rberger_eospac_vectorized

e38cbbe

Yurlungur requested changes Dec 1, 2022

View reviewed changes

rbberger added 2 commits December 5, 2022 16:01

Add constexpr to document Boolean

46ce086

Document unused scratch parameter

9c50fc3

Once we use C++17 this could be replaced with [[maybe_unused]]

rbberger added 3 commits January 11, 2023 09:37

Merge remote-tracking branch 'origin/main' into rberger_eospac_vector…

b4979a7

…ized

Add missing robust::ratio

cd4de7d

Keep LambdaIndexer in EOSPAC vec functions

80a6b03

rbberger force-pushed the rberger_eospac_vectorized branch from 134f642 to 80a6b03 Compare January 11, 2023 18:12

Add EOSPAC vector function python tests

59a2e74

rbberger force-pushed the rberger_eospac_vectorized branch from 791e78e to 59a2e74 Compare January 11, 2023 20:44

Update CHANGELOG.md

50ef5a3

rbberger force-pushed the rberger_eospac_vectorized branch from 6d56ed6 to 50ef5a3 Compare January 11, 2023 21:00

Merge branch 'main' into rberger_eospac_vectorized

61635f6

rbberger force-pushed the rberger_eospac_vectorized branch 3 times, most recently from 8b1907b to b184e19 Compare January 11, 2023 23:11

rbberger added 2 commits January 11, 2023 16:45

Python tests, correct scratch size

b72777c

Add docs about EOSPAC vector API

72a6287

rbberger force-pushed the rberger_eospac_vectorized branch from b184e19 to 72a6287 Compare January 11, 2023 23:45

jhp-lanl approved these changes Jan 12, 2023

View reviewed changes

Yurlungur approved these changes Jan 12, 2023

View reviewed changes

rbberger force-pushed the rberger_eospac_vectorized branch from 6e1ed15 to 72a6287 Compare January 12, 2023 01:19

Merge branch 'main' into rberger_eospac_vectorized

7829ece

rbberger force-pushed the rberger_eospac_vectorized branch from ef4fe7b to 7829ece Compare February 22, 2023 22:02

Yurlungur merged commit fd3e233 into main Feb 23, 2023

Yurlungur deleted the rberger_eospac_vectorized branch February 23, 2023 00:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EOSPAC vector implementations #177

EOSPAC vector implementations #177

rbberger commented Sep 1, 2022 •

edited

Loading

jhp-lanl left a comment

Yurlungur left a comment

Yurlungur commented Sep 1, 2022

rbberger commented Oct 10, 2022

jhp-lanl commented Oct 11, 2022

rbberger commented Oct 11, 2022

Yurlungur commented Oct 11, 2022

Yurlungur commented Oct 12, 2022

rbberger commented Oct 19, 2022

jhp-lanl commented Oct 19, 2022

rbberger commented Oct 21, 2022

jhp-lanl left a comment

Yurlungur left a comment

rbberger commented Jan 11, 2023

jhp-lanl left a comment

jhp-lanl commented Jan 12, 2023

Yurlungur left a comment

jhp-lanl commented Feb 16, 2023

rbberger commented Feb 17, 2023

jhp-lanl commented Feb 17, 2023

rbberger commented Feb 22, 2023

Yurlungur commented Feb 23, 2023

jhp-lanl commented Feb 23, 2023

EOSPAC vector implementations #177

EOSPAC vector implementations #177

Conversation

rbberger commented Sep 1, 2022 • edited Loading

PR Summary

PR Checklist

jhp-lanl left a comment

Choose a reason for hiding this comment

Yurlungur left a comment

Choose a reason for hiding this comment

Yurlungur commented Sep 1, 2022

rbberger commented Oct 10, 2022

jhp-lanl commented Oct 11, 2022

rbberger commented Oct 11, 2022

Yurlungur commented Oct 11, 2022

Yurlungur commented Oct 12, 2022

rbberger commented Oct 19, 2022

jhp-lanl commented Oct 19, 2022

rbberger commented Oct 21, 2022

jhp-lanl left a comment

Choose a reason for hiding this comment

Yurlungur left a comment

Choose a reason for hiding this comment

rbberger commented Jan 11, 2023

jhp-lanl left a comment

Choose a reason for hiding this comment

jhp-lanl commented Jan 12, 2023

Yurlungur left a comment

Choose a reason for hiding this comment

jhp-lanl commented Feb 16, 2023

rbberger commented Feb 17, 2023

jhp-lanl commented Feb 17, 2023

rbberger commented Feb 22, 2023

Yurlungur commented Feb 23, 2023

jhp-lanl commented Feb 23, 2023

rbberger commented Sep 1, 2022 •

edited

Loading