Seacas, Tpetra: fix warning-free build #13192

ikalash · 2024-07-01T22:57:55Z

Not long ago, a warning-free build of Trilinos broke; see #13168. This PR should fix the problem. The changes are to @trilinos/tpetra and @trilinos/seacas .

…ld. See issue #13168.

github-actions · 2024-07-01T22:58:08Z

You seem to have created a PR on master. This is not allowed behavior, so we've blocked your PR. Please switch your PR to target the develop branch and remove the AT: WIP label.

brian-kelley · 2024-07-01T23:03:11Z

packages/tpetra/core/src/Tpetra_CrsGraph_def.hpp

    TEUCHOS_TEST_FOR_EXCEPTION_CLASS_FUNC
-      (numExportLIDs != numPacketsPerLID.extent (0), std::runtime_error,
+      (numExportLIDs !=  numPacketsPerLID_extent, std::runtime_error,


Just out of curiosity, what was the warning here? It looks like it was comparing one extent with another, so they should have both been size_t...

After I changed the type of numExportLIDs to be const LO I got the following error:

projects/albany/nightlyCDashTrilinosBlake/repos-gcc/Trilinos/packages/tpetra/core/src/Tpetra_CrsGraph_def.hpp:5807:42: error: no match for call to '(Kokkos::DualView<long unsigned int*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>, void, void>) (int)' 5807 | (numExportLIDs != numPacketsPerLID(0), std::runtime_error, | ~~~~~~~~~~~~~~~~^~~ /projects/albany/nightlyCDashTrilinosBlake/repos-gcc/Trilinos/packages/teuchos/core/src/Teuchos_TestForException.hpp:180:33: note: in definition of macro 'TEUCHOS_TEST_FOR_EXCEPTION' 180 | const bool throw_exception = (throw_exception_test); \ | ^~~~~~~~~~~~~~~~~~~~ /projects/albany/nightlyCDashTrilinosBlake/repos-gcc/Trilinos/packages/tpetra/core/src/Tpetra_CrsGraph_def.hpp:5806:5: note: in expansion of macro 'TEUCHOS_TEST_FOR_EXCEPTION_CLASS_FUNC' 5806 | TEUCHOS_TEST_FOR_EXCEPTION_CLASS_FUNC

I assume that should say (numExportLIDs != numPacketsPerLID.extent(0) and generate a -Wsign-compare, not this error?

Sorry, in my hasty attempt to manually revert my change, I reverted it incorrectly. The actual error is a sign comparison issue, yes:

/projects/albany/nightlyCDashTrilinosBlake/repos-gcc/Trilinos/packages/tpetra/core/src/Tpetra_CrsGraph_def.hpp:5807:22: error: comparison of integer expressions of different signedness: 'const LO' {aka 'const int'} and 'std::enable_if_t<true, long unsigned int>' {aka 'long unsigned int'} [-Werror=sign-compare]

OK thanks. I would also be OK with just declaring numExportLIDs to be size_t instead, but it doesn't really matter

mperego · 2024-07-02T14:52:47Z

packages/seacas/libraries/ioss/src/exodus/Ioex_DecompositionData.C

@@ -291,10 +291,6 @@ namespace Ioex {
            "exodus", filename, Ioss::READ_RESTART, Ioss::ParallelUtils::comm_self(), properties);
        Ioss::Region region(dbi, "line_decomp_region");

-        int status = Ioss::DecompUtils::line_decompose(


Why are you removing this function call?

ikalash · 2024-07-02T19:04:45Z

For some reason I cannot reply directly to your comment @mperego. I removed int status b/c it is an unused variable within the routine where it appears.

mperego · 2024-07-02T19:20:59Z

For some reason I cannot reply directly to your comment @mperego. I removed int status b/c it is an unused variable within the routine where it appears.

You can still call the function Ioss::DecompUtils::line_decompose and avoid assigning the return value to status.
I don't know what that function does but you are likely changing the behavior of the code if you remove the call.

ikalash · 2024-07-02T19:51:52Z

@mperego interesting. How would you fix the unused variable issue in this case?

brian-kelley · 2024-07-02T20:01:19Z

@ikalash

        (void) Ioss::DecompUtils::line_decompose(
            region, m_processorCount, m_decomposition.m_method, m_decomposition.m_decompExtra,
            element_to_proc_global, INT(0));

The (void) explicitly discards the return value, so that there won't be future warnings about an unused return value

brian-kelley

I didn't pay much attention to the seacase change, thanks @mperego for catching that. I suggested a different change to get rid of the warning.

rppawlo · 2024-07-02T20:27:02Z

seacas is snapshotted into trilinos. Anything you commit to trilinos for seacas will eventually just get overwritten by the next snapshot. @gsjaardema will have to bring the changes back to the main seacas repo.

mperego · 2024-07-02T21:01:26Z

seacas is snapshotted into trilinos. Anything you commit to trilinos for seacas will eventually just get overwritten by the next snapshot. @gsjaardema will have to bring the changes back to the main seacas repo.

Doesn't Seacas team incorporate Trilinos changes before sapshotting, like ROL team does?

rppawlo · 2024-07-02T21:10:34Z

Not that I know of. The ROL team is unique in this regard, as their internal repo is a complete fork of trilinos so it is easy to push either way. The other packages usually use a script to copy modified files - it's a one way operation. Maybe this changed recently - @gsjaardema ?

ikalash · 2024-07-02T23:26:43Z

Thanks for weighing in @rppawlo , @mperego and @brian-kelley . Given the discussion, I think it is probably most expedient to close this PR and to have @gsjaardema and @brian-kelley implement fixes to seacas and tpetra, respectively, as Trilinos developers. @gsjaardema @brian-kelley : would that be OK? I can modify the relevant Trilinos issue which is only about seacas to include tpetra.

brian-kelley · 2024-07-08T22:28:37Z

@ikalash I opened #13213 for the Tpetra changes.

ikalash · 2024-07-08T23:59:27Z

Thank you, @brian-kelley !

gsjaardema · 2024-07-17T18:52:20Z

packages/seacas/libraries/ioss/src/exodus/Ioex_DecompositionData.C

@@ -291,10 +291,6 @@ namespace Ioex {
            "exodus", filename, Ioss::READ_RESTART, Ioss::ParallelUtils::comm_self(), properties);
        Ioss::Region region(dbi, "line_decomp_region");

-        int status = Ioss::DecompUtils::line_decompose(


The correct fix is to either check the status and report an erorr/warning if needed, or to remove just the int status part. The function call is needed.

Yes, I closed my PR b/c there were issues with it. It would be best if you could create a PR with the optimal fix.

Seacas, Tpetra: fixing warnings treated as errors in warning-free bui…

d1d9305

…ld. See issue #13168.

ikalash requested review from a team as code owners July 1, 2024 22:57

github-actions bot added the AT: WIP Causes the PR autotester to not test the PR. (Remove to allow testing to occur.) label Jul 1, 2024

ikalash changed the base branch from master to develop July 1, 2024 22:58

brian-kelley approved these changes Jul 1, 2024

View reviewed changes

mperego reviewed Jul 2, 2024

View reviewed changes

brian-kelley requested changes Jul 2, 2024

View reviewed changes

brian-kelley mentioned this pull request Jul 8, 2024

Tpetra: fix warnings #13213

Merged

ikalash closed this Jul 16, 2024

gsjaardema reviewed Jul 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Seacas, Tpetra: fix warning-free build #13192

Seacas, Tpetra: fix warning-free build #13192

ikalash commented Jul 1, 2024

github-actions bot commented Jul 1, 2024

brian-kelley Jul 1, 2024

ikalash Jul 1, 2024

brian-kelley Jul 1, 2024 •

edited

Loading

ikalash Jul 1, 2024

brian-kelley Jul 2, 2024

mperego Jul 2, 2024

ikalash commented Jul 2, 2024

mperego commented Jul 2, 2024

ikalash commented Jul 2, 2024

brian-kelley commented Jul 2, 2024

brian-kelley left a comment

rppawlo commented Jul 2, 2024

mperego commented Jul 2, 2024

rppawlo commented Jul 2, 2024

ikalash commented Jul 2, 2024

brian-kelley commented Jul 8, 2024

ikalash commented Jul 8, 2024

gsjaardema Jul 17, 2024

ikalash Jul 17, 2024

Seacas, Tpetra: fix warning-free build #13192

Seacas, Tpetra: fix warning-free build #13192

Conversation

ikalash commented Jul 1, 2024

github-actions bot commented Jul 1, 2024

brian-kelley Jul 1, 2024

Choose a reason for hiding this comment

ikalash Jul 1, 2024

Choose a reason for hiding this comment

brian-kelley Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

ikalash Jul 1, 2024

Choose a reason for hiding this comment

brian-kelley Jul 2, 2024

Choose a reason for hiding this comment

mperego Jul 2, 2024

Choose a reason for hiding this comment

ikalash commented Jul 2, 2024

mperego commented Jul 2, 2024

ikalash commented Jul 2, 2024

brian-kelley commented Jul 2, 2024

brian-kelley left a comment

Choose a reason for hiding this comment

rppawlo commented Jul 2, 2024

mperego commented Jul 2, 2024

rppawlo commented Jul 2, 2024

ikalash commented Jul 2, 2024

brian-kelley commented Jul 8, 2024

ikalash commented Jul 8, 2024

gsjaardema Jul 17, 2024

Choose a reason for hiding this comment

ikalash Jul 17, 2024

Choose a reason for hiding this comment

brian-kelley Jul 1, 2024 •

edited

Loading