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 material interface reaction #27737

Merged
merged 14 commits into from
Jun 27, 2024

Conversation

parmar0
Copy link
Contributor

@parmar0 parmar0 commented May 29, 2024

Add a material interface reaction. Closes #27736

Reason

Need to model variable reaction rates.

Design

Adding a class with reaction rate coefficients specified by AD Material Properties.

Impact

Allows users to model variable reaction rates.

@parmar0 parmar0 requested a review from lindsayad as a code owner May 29, 2024 17:48
Copy link
Contributor

@ke7kto ke7kto left a comment

Choose a reason for hiding this comment

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

Good first draft, but I think I see some errors.

@moosebuild
Copy link
Contributor

Job Precheck on 34794f0 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/27737/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 4409da00956c2511d3f6c6db886956a265cd4dd9

@lindsayad
Copy link
Member

You're failing precheck because of trailing whitespace

##########################################################################
ERROR: The following files contain trailing whitespace after applying your patch:
	test/tests/interfacekernels/1d_interface/ADMatreaction_1D_transient.i

Run the "delete_trailing_whitespace.sh" script in your $MOOSE_DIR/scripts directory.

ERROR: The following files do not contain a newline character before EOF:
	test/tests/interfacekernels/1d_interface/ADMatreaction_1D_transient.i

Run the "delete_trailing_whitespace.sh" script in your $MOOSE_DIR/scripts directory.

@parmar0 parmar0 requested a review from cticenhour as a code owner June 3, 2024 17:19
@moosebuild
Copy link
Contributor

moosebuild commented Jun 3, 2024

Job Documentation on c1b574e wanted to post the following:

View the site here

This comment will be updated on new commits.

@ke7kto
Copy link
Contributor

ke7kto commented Jun 3, 2024

@lindsayad I noticed that Akanksha's csvdiff was failing because she wasn't specifying the right csv output file in her test/tests/interfacekernels/1d_interface/ADMatreaction_1D_steady.i. She's put together a fix for that. I had initially thought that the best practice would be to use the same gold file and save space, but is there a chance of a race condition here if we go that route? Should we instead create a new gold csv output - there should be no difference between the reaction_1D_steady and ADMatReaction_1D_steady files.

@lindsayad
Copy link
Member

I usually create a symlink in the gold file directory. This removes the possibility of a race condition

@moosebuild
Copy link
Contributor

moosebuild commented Jun 5, 2024

Job Coverage on c1b574e wanted to post the following:

Framework coverage

d964da #27737 c1b574
Total Total +/- New
Rate 85.06% 85.06% +0.00% 90.91%
Hits 104413 104432 +19 20
Misses 18334 18337 +3 2

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

This comment will be updated on new commits.

Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

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

Please remove the merge commit. You can use rebase if you need something new on next

test/tests/interfacekernels/1d_interface/tests Outdated Show resolved Hide resolved
code with deleted whitespace

addressing errors related to CSV files from review

standardized test output files

adding unique requirement in tests

revised tests to resolve errors
@ke7kto
Copy link
Contributor

ke7kto commented Jun 17, 2024

@lindsayad I think the tests should all be good now, but we have a couple of the modules test that are timing out.

@lindsayad
Copy link
Member

thanks for your contribution @parmar0 !

large_media Outdated Show resolved Hide resolved
large_media Outdated Show resolved Hide resolved
@lindsayad lindsayad changed the title Add a material interface reaction. Closes #27736 Add a material interface reaction Jun 24, 2024
@ke7kto
Copy link
Contributor

ke7kto commented Jun 25, 2024

@lindsayad We've tried to remove any references to the large_media submodule (revert to previous commit, force push), but it seems that we're still trying to change it somehow. We haven't touched that folder. Is this something we need to put more time into, or is there an easy way to remove that from the PR at this point

@cticenhour
Copy link
Member

cticenhour commented Jun 25, 2024

@ke7kto The easiest way at this point, with the current history, is fixing up the commit that contains the update.

cd ~/projects/moose/large_media   # or wherever this MOOSE is
git checkout 2fe4657bc59b93eb8179d22fa1b3ba137361b6b3
cd ..
git add large_media
git commit --fixup 9463948f019e99c8ac39f6613a0135f156e6c1f9
git rebase -i bac2b46800b29699032680442b035c2266eaa99a --autosquash
git push -f origin materialInterfaceReaction  # If "origin" is the name of the remote this is on....change as necessary

EDIT: you might need to cd ~/projects/moose; git submodule update --init large_media prior to cd-ing into the large_media directory

EDIT2: During the autosquash procedure, it'll likely drop you into VIM. Inspect the sequence of commits and if its to your liking, simply :wq.

@ke7kto ke7kto force-pushed the materialInterfaceReaction branch from dbce7fa to edb4651 Compare June 25, 2024 15:19
@cticenhour
Copy link
Member

cticenhour commented Jun 25, 2024

@ke7kto Looks like your last formatting commit didn't apply through the push - did you not see it in the rebase?

EDIT: Good thing is - I don't see the large media submodule in this anymore! 😄

Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

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

thanks for your hard work on this @ke7kto and opening my eyes to these neat interface conditions. Will merge pending tests passing

@lindsayad lindsayad merged commit c5bf9ec into idaholab:next Jun 27, 2024
47 checks passed
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 a material-based interface reaction interfacekernel
5 participants