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

Remove PTofRE implementations from source. Tests pass on my p9 darwin… #293

Merged
merged 7 commits into from
Aug 25, 2023

Conversation

dholladay00
Copy link
Collaborator

@dholladay00 dholladay00 commented Aug 17, 2023

… node.

PR Summary

As discussed with @Yurlungur, this function was the sole reason for very excessive GPU compile times. The singular place it was used was replaced with an equivalent functionality. It does not appear to be used anywhere else.

This PR removes PTofRE function. I'd like to hear from folks downstream, @jdolence @jhp-lanl do you foresee issues with the removal of this function to downstream use cases and/or future use cases (PT table equivalent for example @jhp-lanl?)

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
  • If preparing for a new release, update the version in cmake.

@dholladay00 dholladay00 requested a review from Yurlungur August 17, 2023 15:54
@Yurlungur
Copy link
Collaborator

To add some context, this function was originally written as a convenience for the flag-type PTE solver. But the current solvers don't use this function, and the same functionality is available as a collection of calls, or through FillEos.

While I believe we may need functions that compute quantities given Pressure as an input, this function doesn't provide it. So I see this as just removing code that (a) we don't need and shouldn't continue to maintain and (b) apparently slows compile times way down for some reason.

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

The list of files changed doesn't seem like the entire list of Eoses and modifiers. Let's just make sure we got them all.

@dholladay00
Copy link
Collaborator Author

The list of files changed doesn't seem like the entire list of Eoses and modifiers. Let's just make sure we got them all.

I found things to remove via grep, so I think things that didn't include probably relied on the base implementation.

@jhp-lanl
Copy link
Collaborator

Obviously not a blocker for this MR: I'd like to understand why this was so bad at some point though so that we know what we can't feasibly do in the CRTP base class. Like is it a problem to call multiple class member functions there? Or is it a combination of that with the parallel for in the vector version? I suppose we'll find out whenever we try something that drastically increases compilation times 🙃

Copy link
Collaborator

@jhp-lanl jhp-lanl left a comment

Choose a reason for hiding this comment

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

I think you left a dangling remnant of the documentation. Otherwise looks good

doc/sphinx/src/using-eos.rst Outdated Show resolved Hide resolved
@Yurlungur
Copy link
Collaborator

@dholladay00 is this ready to go? I re-triggered the re-git pipelline

@dholladay00
Copy link
Collaborator Author

It should be, the reason the pipeline had failed was due to a timeout due to the inability to obtain an x86-v100 node in a timely manner.

@Yurlungur
Copy link
Collaborator

Tests passing. Pulling the trigger.

@Yurlungur Yurlungur merged commit 0584cf9 into main Aug 25, 2023
4 checks passed
@Yurlungur Yurlungur deleted the dholladay00/remove_ptre branch August 25, 2023 16:35
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