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

Reintroduce old VectorTools::interpolate as fallback #5334

Merged
merged 2 commits into from Oct 26, 2017

Conversation

gassmoeller
Copy link
Member

As discussed in #5261 and geodynamics/aspect#1936 (and I am sure in other places as well), the current version of VectorTools::interpolate fails for FESystems that contain elements without generalized support points (even when we do not want to interpolate their components). This is a major issue for the ASPECT community, because in about half of our models we rely on these systems. For us as developers that means we can not longer test the current ASPECT version with the current deal.II version, and we also had users trying to update their deal.II, and wasting their time figuring out what went wrong. Thus, we had to issue a warning to all users to not update deal.II until this problem is fixed.

I spend half a day yesterday thinking about how to fix the function, but I did not come up with a nice solution. There are several places in the function that rely on the existence of generalized support points for the whole system, and even worse some functions that are called inside do the same. A reorganization of the function as a loop over all base elements or components seems ugly and inefficient, and would essentially be a rewrite of the whole function. Since I currently do not have the time to dig into this issue in depth, and the matter is of a certain urgency to me, I want to propose to reintroduce the old version of the function as a fallback for cases in which the new version does not work. I am aware that code duplication is a bad thing, and this is not meant as a solution, but as a hotfix that allows us to continue our development in ASPECT (and me to go forward with #5125). If somebody has suggestions for how to actually fix the issue I am open to them, but even then I would prefer to hotfix this issue now (with this PR), and work on a solution in a separate PR. I reinserted a failing test that now works again that exactly illustrates the problem.

For reference, the version of the function I introduce here is the one that previously existed in commit 60bfd31 (I had to change the type of the interpolated function and all calls to it, and move one assert, but the rest is identical).

@bangerth
Copy link
Member

@tamiko?

@gassmoeller -- you'll have to run the indentation script again.

@tamiko
Copy link
Member

tamiko commented Oct 26, 2017

I am not convinced that the solution should be to duplicate the interpolate function again.

There is a relatively clean solution that would require to rewrite our logic for generalized support points a bit but will support systems with non-selected non-interpolatory components by simply ignoring such components in the recursive procedure in FESystem.
I plan to work on this next week.

@tamiko
Copy link
Member

tamiko commented Oct 26, 2017

If it is really that urgent to get a hotfix in for a week, so be it.

Please reactivate the corresponding test (that has been moved to fail/interpolate_q_system_mixed_01.cc), though.

@bangerth
Copy link
Member

If you can promise that that will happen, then that's something we can live with. But the pre-existing functionality has been broken for several weeks already, and we need to eventually come to a resolution that works for those of us who used that functionality.

@tamiko
Copy link
Member

tamiko commented Oct 26, 2017

@bangerth I was not aware of the fact that this broke some functionality in aspect.

@davydden
Copy link
Contributor

I am not convinced that the solution should be to duplicate the interpolate function again.

I agree with @tamiko that if there is a clean way to fix it, this should be done as opposed to pulling back the removed code.

@bangerth
Copy link
Member

I think that's what we all want, but @gassmoeller has been asking for a fix for weeks already.

@gassmoeller
Copy link
Member Author

I would still really prefer if we could move forward with this PR.

@davydden: To explain my changes better: I do not remove the current version. I only add an if at the beginning that redirects to the old version in case the current version would crash. It would be very simple to revert this PR, either as part of the final fix, or I would also be happy to do it myself when the solution has been implemented (simply remove the if, and the old function).

@tamiko: I do not doubt that you can find a nicer solution to the issue, and if it was just one week that would be no issue. But since this was broken for a while (probably bad communication on our side to not let you know), and with new PRs you never know how long discussion takes (or if there are unexpected complications), I would just like to have a workaround available right now, and work on the real solution with all the time that is necessary to make a good solution, without time pressure.

@masterleinad
Copy link
Member

/run-tests

@drwells
Copy link
Member

drwells commented Oct 26, 2017

I was also not aware that 'this has been broken for weeks'. Regardless, I agree with @tamiko and @gassmoeller that we should add this temporary fix now (and @tamiko has already volunteered to fix everything next week :)).

@drwells
Copy link
Member

drwells commented Oct 26, 2017

Since this is a fairly serious regression I would prefer to merge it as soon as possible. I will do so later today unless someone objects.

@tjhei tjhei merged commit fc1051f into dealii:master Oct 26, 2017
@tjhei
Copy link
Member

tjhei commented Oct 26, 2017

Looks good to me. Let's hope @tamiko will be back soon. ;-)

@gassmoeller
Copy link
Member Author

Thanks all!

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

7 participants