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.

Sign up for GitHub

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

Jump to bottom

[ConvDIff] Add flag to test json files #7638

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

[ConvDIff] Add flag to test json files #7638

wants to merge 7 commits into from
+20 −9

Conversation

riccardotosi
Copy link
Collaborator

Description
Add time_integration_method key to json files of tests.

Please mark the PR with appropriate tags:

  • Applications, FastPR, Testing

Changelog
See description.

@riccardotosi riccardotosi added Applications Testing FastPR This Pr is simple and / or has been already tested and the revision should be fast labels Oct 21, 2020
@riccardotosi riccardotosi requested a review from a team as a code owner October 21, 2020 06:59
@philbucher
Copy link
Member

it it enforced now to have this flag?

@riccardotosi
Copy link
Collaborator Author

it it enforced now to have this flag?

@rubenzorrilla asked me to add to avoid printing warnings when running tests.

@philbucher
Copy link
Member

hm in my opinion (and in accordance with StructuralMechanics) I would not add a warning if this is missing, by default implicit is used
In structural however the explicit is not so much used, most of the times it is implicit. Not sure what your plans are here, up to you :)

rubenzorrilla
rubenzorrilla reviewed Oct 21, 2020
View reviewed changes
...ctionDiffusionApplication/tests/thermo_mechanical_tests/coupled_problem_test_parameters.json Outdated
@@ -27,7 +27,6 @@
"time_stepping" : {
"time_step" : 0.4
},
"time_integration_method" : "implicit",
Copy link
Member

Choose a reason for hiding this comment

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

This is not the time_integration_method for the mechanical problem but of the conv-diff one. Please keep it. In this case I'd keep the warning to clearly inform the user of which integration method is used by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was getting an error in the CI with this. This is why I removed it. What you propose is to revert the last commit "Revert flag"
d533805 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me push again so we can see the error

Copy link
Member

Choose a reason for hiding this comment

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

It seems that you have an error in the json sintax

@rubenzorrilla
Copy link
Member

You should add the new field to the defaults of the solver. Otherwise it crashes when checking.

@riccardotosi
Copy link
Collaborator Author

@rubenzorrilla what are we waiting for to merge?

@rubenzorrilla
Copy link
Member

@rubenzorrilla what are we waiting for to merge?

The GUI update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubenzorrilla rubenzorrilla rubenzorrilla left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Applications FastPR This Pr is simple and / or has been already tested and the revision should be fast Testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@riccardotosi @philbucher @rubenzorrilla