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

Fix enabling of Use Serial Mesh #762

Merged
merged 2 commits into from
Nov 16, 2021
Merged

Fix enabling of Use Serial Mesh #762

merged 2 commits into from
Nov 16, 2021

Conversation

mperego
Copy link
Collaborator

@mperego mperego commented Nov 15, 2021

Check whether parallel ExodusII is available and use it to conditionally enable Use Serial Mesh. Thanks @bartgol for helping.

@mperego mperego requested a review from bartgol November 15, 2021 22:13
@mperego
Copy link
Collaborator Author

mperego commented Nov 15, 2021

This is related to #761.

@mperego mperego changed the title Check whether ExodusII_par is available and use it to configure enabl… Fix enabling of Use Serial Mesh Nov 15, 2021
@ikalash
Copy link
Collaborator

ikalash commented Nov 15, 2021

Thanks @mperego ! I'll test this out on camobap later this afternoon.

@mperego
Copy link
Collaborator Author

mperego commented Nov 15, 2021

@ikalash Thanks!
B.t.w. It appears that a couple of tests

CahnHillElast2D_Epetra
CahnHillElast2D_Tempus

were if-guarded by Albany_IOPX and were never enabled. This PR enables them and they are now failing:

Error, the parameter {name="Initial Order",type="int",value="0"}
in the parameter (sub)list "Albany Parameters->Piro->Tempus->Tempus Integrator->Time Step Control"
was not found in the list of valid parameters!

Copy link
Collaborator

@ikalash ikalash left a comment

Choose a reason for hiding this comment

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

Could you please add a print of whether ALBANY_PARALLEL_EXODUS is true/false in the CMakeLists.txt file? Would be useful to see this in the configure step, for debugging, etc.

@ikalash
Copy link
Collaborator

ikalash commented Nov 15, 2021

@ikalash Thanks! B.t.w. It appears that a couple of tests

CahnHillElast2D_Epetra
CahnHillElast2D_Tempus

were if-guarded by Albany_IOPX and were never enabled. This PR enables them and they are now failing:

Error, the parameter {name="Initial Order",type="int",value="0"}
in the parameter (sub)list "Albany Parameters->Piro->Tempus->Tempus Integrator->Time Step Control"
was not found in the list of valid parameters!

Thanks for the heads up - I'll fix this.

CMakeLists.txt Outdated
return 0;
}
"
PARALLEL_EXODUS_SUPPORTED
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason not to just use ALBANY_PARALLEL_EXODUS as output variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really. I thought that
Performing Test PARALLEL_EXODUS_SUPPORTED - Success
was more expressive but at the same time wanted to keep Albany way of naming variables. I can change that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it's fine. I was just curious if there was some weird techincality in the CheckFunctionExists macro maybe. This is fine.

Copy link
Collaborator

@bartgol bartgol left a comment

Choose a reason for hiding this comment

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

Looks good. I have one comment, but not very important.

@mperego
Copy link
Collaborator Author

mperego commented Nov 15, 2021

Could you please add a print of whether ALBANY_PARALLEL_EXODUS is true/false in the CMakeLists.txt file? Would be useful to see this in the configure step, for debugging, etc.

Yes, what message would you like to print? I don't see it done in a consistent way. Something like
ALBANY_PARALLEL_EXODUS set to True ?

@ikalash
Copy link
Collaborator

ikalash commented Nov 15, 2021

Could you please add a print of whether ALBANY_PARALLEL_EXODUS is true/false in the CMakeLists.txt file? Would be useful to see this in the configure step, for debugging, etc.

Yes, what message would you like to print? I don't see it done in a consistent way. Something like ALBANY_PARALLEL_EXODUS set to True ?

Yes exactly - to be consistent with the other messages printed when you configure Albany.

@ikalash
Copy link
Collaborator

ikalash commented Nov 16, 2021

This looks good. I just verified that everything is OK on camobap with your change. I'll go ahead and merge.

@ikalash ikalash merged commit 2e3f210 into master Nov 16, 2021
@ikalash ikalash deleted the config_serial_mesh branch November 16, 2021 00:55
@mperego
Copy link
Collaborator Author

mperego commented Nov 16, 2021

@ikalash thanks for checking camobap. I usually prefer to rebase and merge to avoid useless merge commits.

B.t.w. the failing CahnHill tests don't appear on Camobap because ALBANY_PARALLEL_EXODUS is false there.

@ikalash
Copy link
Collaborator

ikalash commented Nov 16, 2021

@mperego , yes, thank you for the reminder. I'll fix it in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants