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

Surfchem heatflux #278

Merged
merged 31 commits into from Oct 29, 2023
Merged

Surfchem heatflux #278

merged 31 commits into from Oct 29, 2023

Conversation

aborner1
Copy link
Contributor

Purpose

New feature: add an option to "compute surf" to compute the chemical energy due to catalytic surface reactions (such as exothermic recombination reactions). Also now take into account that chemical energy in the total energy,

Author(s)

Arnaud Borner (NASA Ames)

Backward Compatibility

Backwards compatible, except that a new surface chemistry file needs to be used,.

Implementation Notes

Added the heat of reaction to the surface reaction input file, added "echem" as an option to "compute surf", and added the chemical surface catalytic energy to "etot". Edited doc.

Post Submission Checklist

Please check the fields below as they are completed

  • The feature or features in this pull request is complete
  • Suitable new documentation files and/or updates to the existing docs are included
  • One or more example input decks are included
  • The source code follows the SPARTA formatting guidelines

@aborner1 aborner1 force-pushed the surfchem-heatflux branch 2 times, most recently from 0f1af0e to e94b8a1 Compare February 8, 2022 21:53
Merge master into surfchem-heatflux
Quite a few changes needed to port to current master since surf_adsorb was implemented.
whitespace/newline
make sure transparent case is handled properly
@aborner1
Copy link
Contributor Author

aborner1 commented Feb 9, 2022

I have merged current master into this and it's passing all the reg tests. It should be ready to merge. Can we take care of it soon before it needs more manual merging?

@stanmoore1 stanmoore1 requested a review from sjplimp March 1, 2022 20:40
@stanmoore1
Copy link
Contributor

@aborner1 I asked @sjplimp to review.

@aborner1
Copy link
Contributor Author

@stanmoore1 @sjplimp Ditto here: Talked to Michael and he green-lighted this. Can we proceed with review and merge?

@stanmoore1
Copy link
Contributor

@aborner1 the changes here need to be ported to Kokkos, should have time to work on it this week.

@stanmoore1 stanmoore1 added the enhancement New feature or request label May 17, 2022
@sjplimp
Copy link
Contributor

sjplimp commented Jul 11, 2022

@aborner1 I added support for 4 other surf collide styles that take Twall as in input parameter
@stanmoore1 you can look this over - I think it may affect Kokkos versions of surf collide styles

@aborner1
Copy link
Contributor Author

aborner1 commented Dec 9, 2022

@stanmoore1 Have you had a chance to look at the Kokkos version of this command?

@stanmoore1 stanmoore1 assigned stanmoore1 and unassigned sjplimp Dec 9, 2022
@stanmoore1
Copy link
Contributor

@aborner1 I'm porting this PR to Kokkos now. Same as the other PR, if you have an example that would be helpful. Thanks

@aborner1
Copy link
Contributor Author

aborner1 commented Apr 7, 2023

@stanmoore1 Yes, I can very easily do that for this one, super simple.

@aborner1
Copy link
Contributor Author

@stanmoore1 Yes, I can very easily do that for this one, super simple.

done

@stanmoore1
Copy link
Contributor

Thanks @aborner1!

@stanmoore1 stanmoore1 self-requested a review as a code owner June 20, 2023 19:20
@stanmoore1
Copy link
Contributor

@aborner1 this is nearly ready to merge. One loose end I see is that the new feature only applies to the prob style surf reaction, global and adsorb are ignored. I think this should be mentioned in the docs.

@aborner1
Copy link
Contributor Author

@aborner1 this is nearly ready to merge. One loose end I see is that the new feature only applies to the prob style surf reaction, global and adsorb are ignored. I think this should be mentioned in the docs.

Agreed. Are you able to add a quick mention while you are working on this?

@stanmoore1
Copy link
Contributor

Yes will do

@stanmoore1
Copy link
Contributor

@aborner1 It also looks like this is going to force all SPARTA reaction files to have 2 coefficients instead of 1, correct? It seems like it may be better to have this optional in the reaction file, i.e. default the second coeff to 0.0. @sjplimp do you agree?

@aborner1
Copy link
Contributor Author

@aborner1 It also looks like this is going to force all SPARTA reaction files to have 2 coefficients instead of 1, correct? It seems like it may be better to have this optional in the reaction file, i.e. default the second coeff to 0.0. @sjplimp do you agree?

That is correct, and I am fine with defaulting to a 0.0 value, which would replicate the previous behavior.

@stanmoore1 stanmoore1 merged commit 3e48042 into sparta:master Oct 29, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants