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

Fixes for staggering in lapd-drift example #1925

Merged
merged 1 commit into from
Mar 2, 2020
Merged

Conversation

johnomotani
Copy link
Contributor

Also replaces deprecated Grad_par_CtoL() and Grad_par_LtoC() operators.

Also replaces deprecated Grad_par_CtoL() and Grad_par_LtoC() operators.
@johnomotani johnomotani added backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 bugfix labels Feb 28, 2020
@ZedThree
Copy link
Member

ZedThree commented Mar 2, 2020

@ZedThree, @bendudson The staggered derivative operators in lapd_drift.cxx were being used the opposite way round to what I thought they meant. Grad_par_CtoL() was taking a flux (which I expected to be at CELL_YLOW) as argument, and returning a result for a scalar equation (which I expected to be at CELL_CENTRE), I thought it should be the other way around: i.e. CtoL meant 'argument at C' and 'result at L'. Doesn't really matter now that the CtoL and LtoC operators are deprecated anyway, but noting here in case issues come up converting old physics models.

Originally posted by @johnomotani in #1923 (comment)

I double checked the old implementation of the LtoC/CtoL operators, and they did work as you thought. A little bit worrying! Checking a couple of other old physics models, and they are mostly using them the correct way round. I think only examples/tokamak-2fluid/2fluid.cxx and examples/elm-pb/elm-pb.cxx are also backwards.

@johnomotani Please could you check those models too?

EDIT: most of the examples that use the LtoC/CtoL operators don't actually call setLocation, so they're probably all a little bit dubious

@ZedThree ZedThree merged commit 0b334e6 into next Mar 2, 2020
@ZedThree ZedThree deleted the lapd-drift-fixes branch March 2, 2020 15:33
@johnomotani
Copy link
Contributor Author

@johnomotani Please could you check those models too?

EDIT: most of the examples that use the LtoC/CtoL operators don't actually call setLocation, so they're probably all a little bit dubious

I guess originally as long as the model was consistent with itself it didn't matter much. Having LtoC/CtoL 'back to front' would just correspond to having the scalars at CELL_YLOW and fluxes at CELL_CENTRE. I think now it would make sense to update all of them to the current conventions, ignoring which way round CtoL/LtoC were originally. I'll make a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants