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

Add a cvode switch on the ordering #383

Merged
merged 23 commits into from
Oct 30, 2023
Merged

Conversation

marchdf
Copy link
Contributor

@marchdf marchdf commented Apr 19, 2023

As it is, it breaks sparse direct. Would be nice to make the ordering a runtime variable...

@marchdf marchdf requested review from esclapez and jrood-nrel April 19, 2023 19:16
@marchdf marchdf marked this pull request as draft April 24, 2023 18:36
@jrood-nrel jrood-nrel removed the request for review from esclapez July 26, 2023 16:49
@jrood-nrel
Copy link
Contributor

What if we add an abort to this in everything but GMRES. Can we do a runtime check on the ordering?

@jrood-nrel jrood-nrel marked this pull request as ready for review October 27, 2023 17:10
@jrood-nrel jrood-nrel requested a review from baperry2 October 27, 2023 17:14
@jrood-nrel
Copy link
Contributor

This is like a 10% performance increase in total simulation time. The option PELE_CVODE_FORCE_YCORDER opens up the full set of solvers for any advanced users, but I think we should automatically default CYOrder when possible.

@baperry2
Copy link
Contributor

I'm ok with this being a compile time option, but I think the behavior here is kind of confusing. If I understand properly, this basically means a user can't choose anything besides GMRES or MAGMA on GPU without modifying source code, because the PELE_CVODE_FORCE_YCORDER define is not exposed to the user though the GNUmakefile in the way that for example PELE_USE_MAGMA is. I think it would be better for users to control this more directly.

Unfortunately, I think the documentation on the chemistry integrators is so out of date that it's not even possible to add an explanation there. We really do need to address that at some point, as right now its very difficult for users to know which combinations of YC/CYOrder, EOS, hardware and solve type are valid, let alone what might be expected to be the fastest.

@jrood-nrel
Copy link
Contributor

In each Pele code we would add PELE_CVODE_FORCE_YCORDER which is simple, just like PELE_USE_MAGMA.

@baperry2
Copy link
Contributor

Shouldn't that be added to Make.PelePhysics as well?

@jrood-nrel jrood-nrel merged commit 5097588 into development Oct 30, 2023
8 checks passed
@jrood-nrel jrood-nrel deleted the cvode-order-switch branch October 30, 2023 19:43
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