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

Removed interface compatibility. #1700

Merged
merged 2 commits into from May 19, 2017
Merged

Removed interface compatibility. #1700

merged 2 commits into from May 19, 2017

Conversation

alarshi
Copy link
Contributor

@alarshi alarshi commented May 16, 2017

In benchmarks/solkz

@bangerth
Copy link
Contributor

/run-tests

@tjhei
Copy link
Member

tjhei commented May 16, 2017

It looks correct, so I am surprised that the output changes. I guess we ignore this slight change and update the results?

@bangerth
Copy link
Contributor

@tjhei: Even though this patch changes a file in benchmarks/, are there tests that reference the material model?

@bangerth
Copy link
Contributor

@tjhei -- no, the output shows errors, and they are a factor of 5-10 smaller than before. We need to figure out what is different here before this can be merged. Something is substantially wrong here.

@tjhei
Copy link
Member

tjhei commented May 16, 2017

@tjhei: Even though this patch changes a file in benchmarks/, are there tests that reference the material model?

Yes. We include the .cc in the test.

@tjhei -- no, the output shows errors, and they are a factor of 5-10 smaller than before.

One additional Stokes iteration can cause such a difference (errors are small to begin with).

@bangerth
Copy link
Contributor

Really? This is crazy. What you're saying is that the total error is completely determined by the iteration error, not the discretization error.

@bangerth
Copy link
Contributor

Addresses #1538.

@tjhei
Copy link
Member

tjhei commented May 16, 2017

Well, not 100% sure...

@bangerth
Copy link
Contributor

Let's leave this for some later time after the hackathon to investigate. We're not going to get to the bottom of it tonight.

@gassmoeller
Copy link
Member

Found the problem. The particle_interpolator_bilinear_solkz test uses a material model that is derived from the SolKzMaterial class, which changed in this PR. The derived model overloads the density and viscosity functions and modifies their behavior. Because of this PR these functions are not longer called, and therefore the modified behavior is never triggered. @alarshi: Please keep this PR as is, except that the new class still needs the density and viscosity function exactly as they were before. evaluate then needs to call these functions to determine the density and viscosity.

@tjhei
Copy link
Member

tjhei commented May 19, 2017

I did the fix.

@bangerth
Copy link
Contributor

Yes, @tjhei's is the way to go. But can you (@tjhei) run astyle on your patch?

OK to merge if the tester is happy.

@tjhei
Copy link
Member

tjhei commented May 19, 2017

Beginner mistake. ;-)

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.

None yet

4 participants