Add Cardinal to FENIX #53
Conversation
|
Note that some changes to the FENIX recipes need to occur before this can be merged. I am working on a recipe PR that I will get reviewed and merged next week. |
|
All - also happy to take feedback on the script configuration currently used here. An alternative is doing everything in the Makefile, which should remove at least one step from the process as currently outlined, and require fewer recipe changes. |
|
Thank you Casey! I was able to follow your instructions and install FENIX with Cardinal enabled. I ran Cardinal test suite as well and got 384 passed, 254 skipped, 0 pending, 6 FAILED. The failed tests are probably out of the scope of this PR (i.e. one of them uses THM which we're not including here). otherwise it looks great to me! |
There was a problem hiding this comment.
Looks good to me. Thank you @cticenhour for adding this!
I'll re-review after the new FENIX recipe is added.
|
Converting as draft until we get the new recipe merged. |
|
I think it would be nice to get @loganharbour's review as well. |
@meltawila Could also be related to our MOOSE submodule being ahead of Cardinal's; that would be my guess with only a few tests failing. Thanks for checking with your workflow! |
|
With the most-recent documentation commit, a small adjustment to TMAP8 is now required. idaholab/TMAP8#153 is pending review and merge. |
|
Based on a discussion today with @loganharbour, I think I will adjust this script slightly and the FENIX |
|
One last adjustment to MOOSE was required, which has now been merged. Once that makes it to the MOOSE submodule, I think we'll be ready to go. |
|
I think the desired submodule update was not done yet |
cticenhour
force-pushed
the
add_cardinal
branch
from
693451c
to
d1b0527
Compare
|
@meltawila However, I don't think we need to add the update and rebuild instructions. They should be provided with the latest |
|
Job Precheck on 7b654de wanted to post the following: Your code requires style changes. A patch was auto generated and copied here
Alternatively, with your repository up to date and in the top level of your repository:
|
cticenhour
force-pushed
the
add_cardinal
branch
2 times, most recently
from
c41ae36
to
c10bb9a
Compare
cticenhour
force-pushed
the
add_cardinal
branch
from
c10bb9a
to
a134ecb
Compare
Default for Cardinal is changed to 'yes' instead of 'no'
Co-authored-by: Pierre-Clement Simon <[email protected]>
This is also reflected in the HPC instructions. Finally, improve spacing for warning message when Cardinal is disabled.
Co-authored-by: Pierre-Clement Simon <[email protected]>
There was a problem hiding this comment.
Looks great to me, and this is a great contribution! Thank you @cticenhour!!
|
@loganharbour, do you want to give this a another review before we merge it? |
|
@loganharbour I had already updated the follow-on recipe PR to include it if you want to do it there, or I can pull it out separately. EDIT: We're doing it separately, idaholab/civet_recipes#1874 |
Refs idaholab#52 This aligns with the status of Cardinal, which was also made optional in idaholab#53
Refs idaholab#52 This aligns with the status of Cardinal, which was also made optional in idaholab#53
Refs idaholab#52 This aligns with the status of Cardinal, which was also made optional in idaholab#53
Closes #38
@meltawila As discussed earlier, please build this version of FENIX and run it against your Cardinal inputs from #30 (and any other inputs you are working on, as you like). Please also add any comments you have on build instructions and usage issues to this thread so that they can be addressed as part of the review process. Thanks!
Requesting @simopier for the formal review once this test run is complete and any comments from that test are fully addressed.