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 Thermochimica submodule and coupling code #22712

Merged
merged 13 commits into from
Dec 15, 2022

Conversation

dschwen
Copy link
Member

@dschwen dschwen commented Nov 15, 2022

Closes #22711

@dschwen
Copy link
Member Author

dschwen commented Nov 15, 2022

Some comments

  • Only one object per MOOSE simulation may initialize Thermochimica and set the database file (this is currently enforced by providing an action class that has a syntax that can only be instantiated once)
  • No threaded runs (there is just on global state in Thermochimica)
  • I'm planning to change the nodal user object to utilize writeableCoupledValue (this will require another PR that I'm working on)
  • Thermochimica has an internal buffer for the data base path and filename that has a size limit of 120 chars. I already ran into this limit and had to adjust the tests dir structure accordingly (@maxposchmann maybe we can raise the limit or even make it dynamic...)

@dschwen
Copy link
Member Author

dschwen commented Nov 16, 2022

Is Relap-7 currently broken?

@dschwen
Copy link
Member Author

dschwen commented Nov 16, 2022

Note to future self: we could support threaded execution and multiple Thermochimica instances with a wrapper that uses fork() to create child process copies of the running MOOSE executable. We'd set up pipes to communicate with each child. On Linux fork uses a copy-on-write memory model, so the child processes would be super lightweight. The children would remain in a read() loop that listens on the pipe for RPC calls to that get translated to Thermochimica API calls. Calculation results would be sent back through the pipe. Since the thermodynamic solves are pretty costly the RPC overhead is probably negligible, and it would allow us to take advantage of threading.

@moosebuild
Copy link
Contributor

moosebuild commented Nov 16, 2022

Job Documentation on c9a7b7d wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

moosebuild commented Nov 16, 2022

Job Coverage on c9a7b7d wanted to post the following:

Framework coverage

0674f2 #22712 c9a7b7
Total Total +/- New
Rate 84.51% 84.51% -0.00% -
Hits 81224 81222 -2 0
Misses 14891 14893 +2 0

Diff coverage report

Full coverage report

Modules coverage

Chemical reactions

0674f2 #22712 c9a7b7
Total Total +/- New
Rate 92.84% 92.79% -0.05% 92.61%
Hits 869 1107 +238 238
Misses 67 86 +19 19

Diff coverage report

Full coverage report

Full coverage reports

Reports

This comment will be updated on new commits.

.gitmodules Outdated Show resolved Hide resolved
@loganharbour
Copy link
Member

In most cases we don't update --init any submodules in moose in CI because we use modules for petsc and libmesh. They're only updated on the fly as needed. With this, I need to go add this submodule to each recipe that needs it. Which is fine... except I need to do some hackery to only init it if it's available

@maxposchmann
Copy link
Contributor

maxposchmann commented Nov 16, 2022

  • Thermochimica has an internal buffer for the data base path and filename that has a size limit of 120 chars. I already ran into this limit and had to adjust the tests dir structure accordingly (@maxposchmann maybe we can raise the limit or even make it dynamic...)

@dschwen, if you have a suggestion for how to make this dynamic, please feel free to open another PR at Thermochimica.

Edit: I opened ORNL-CEES/thermochimica#109 for this purpose

@moosebuild
Copy link
Contributor

Job App documentation on fe96d22 : invalidated by @joshuahansel

RELAP-7 fixed

@dschwen
Copy link
Member Author

dschwen commented Nov 23, 2022

Ugh, in this version of the PR I forgot to transfer the doc files. I'll have to fix that when I'm back next week.

@moosebuild
Copy link
Contributor

All jobs on 2534e41 : invalidated by @loganharbour

Re-run now that we checkout the submodule

@moosebuild
Copy link
Contributor

moosebuild commented Nov 24, 2022

Job Test timings on c9a7b7d wanted to post the following:

View timings here

This comment will be updated on new commits.

Copy link
Contributor

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

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

see comments on Saline PR on the Makefile

@moosebuild
Copy link
Contributor

Job Precheck on 2847fcc : invalidated by @dschwen

@dschwen dschwen force-pushed the thermochimica_22711 branch 5 times, most recently from 0a99b56 to 9eac92b Compare December 8, 2022 17:04
@dschwen
Copy link
Member Author

dschwen commented Dec 11, 2022

Everything passed and is green (if you click through to Civet). Looks like the status update on Github is just not working right.

@GiudGiud
Copy link
Contributor

It seems. i think a few comments did not get addressed too

GiudGiud
GiudGiud previously approved these changes Dec 13, 2022
@dschwen dschwen force-pushed the thermochimica_22711 branch 2 times, most recently from 65cc1a8 to 0ecb5fa Compare December 14, 2022 05:18
GiudGiud
GiudGiud previously approved these changes Dec 14, 2022
@dschwen dschwen merged commit 5c9e6aa into idaholab:next Dec 15, 2022
cticenhour added a commit to cticenhour/moose that referenced this pull request Dec 23, 2022
cticenhour added a commit to cticenhour/moose that referenced this pull request Dec 23, 2022
bwspenc pushed a commit to bwspenc/moose that referenced this pull request Dec 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Thermochimica as an optional submodule
6 participants