Deprecate mergeYupYdown(), rename splitYupYdown() to createYupYdown()

[johnomotani]

splitYupYdown() is still present, but also deprecated.

These are deprecated so we can remove them in v4.3, see PR #1287.

Moving ParalllelTransformIdentity::calcYupYdown() to a .cxx file, because otherwise 'make' gets noisy due to paralleltransform.hxx being included in most files.

[ZedThree]

I'm still not sure. Removing mergeYupYdown implies that it will never be useful to remove the yup/ydown fields. It might have a use for debugging or performance?

From #1287:

Removing the method simplifies several conditionals, which can now be just 'var.hasYupYdown()' where previously they also had to check 'var.yup() != var' and 'var.ydown() != var'.

If I understand, that is really the purpose of this PR, simplifying those conditionals? Is an alternative to just move the full checks into Field3D::hasYupYdown?

Also, deprecating something implies we should be able to completely delete all uses of it from the library. Now that we're -Wall clean, I'd really like to try and keep make free from all warnings if possible!

[johnomotani]

mergeYupYdown() makes yup and ydown point to the field itself, so I think having the method at all only makes sense for ParallelTransformIdentity. I think we can remove it entirely if ParallelTransformIdentity just returns false for hasYupYdown(); mergeYupYdown is only used in 1 or 2 places, for special handling of the ParallelTransformIdentity case:

BOUT-dev/src/mesh/coordinates.cxx

Lines 707 to 720 in 8bb7ce6

	if (f.hasYupYdown()) {
	// Need to modify yup and ydown fields
	Field3D f_B = f / Bxy_floc;
	if (&f.yup() == &f) {
	// Identity, yup and ydown point to same field
	f_B.mergeYupYdown();
	} else {
	// Distinct fields
	f_B.splitYupYdown();
	f_B.yup() = f.yup() / Bxy_floc;
	f_B.ydown() = f.ydown() / Bxy_floc;
	}
	return Bxy * Grad_par(f_B, outloc, method);
	}

and Div_par_flux on #1318.

If we want to remove the yup/ydown fields, I think we should have a different method, like deleteYupYdown(), which just deletes them. e.g. from #1177

BOUT-dev/src/field/field3d.cxx

Lines 204 to 220 in 2dd1c1d

	void Field3D::deleteYupYdown() {
	// Delete auxiliary fields if they have been set
	if (!(yup_field == this && ydown_field == this)) {
	delete yup_field;
	yup_field = nullptr;
	delete ydown_field;
	ydown_field = nullptr;
	}
	if (field_fa != this) {
	delete field_fa;
	}
	field_fa = nullptr;
	has_field_aligned = false;
	}

[ZedThree]

After a bit more thinking and some discussion with @d7919, I think this is hiding the real issue which is that we still haven't implemented multiple yup fields.

If we handle the identity transform using the field-aligned branches, we'll be doing two unnecessary copies each time (although copy-on-write will reduce the pain of this a lot).

Sorting out the multiple yup fields should be a priority for 4.3.

[johnomotani]

Ah, I hadn't twigged the copying issue. Makes sense to me now to keep mergeYupYdown() and just wait for implementing multiple yup fields to simplify the conditionals.

It's annoying that there doesn't seem to be a way of avoiding copies when using toFieldAligned (although I may just be missing it, not being very good with c++11). The compiler will avoid a copy when returning a temporary value with return type Field3D (as in ShiftedMetric::toFieldAligned). We can avoid a copy by returning a Field3D& reference if there's a persistent variable to return (as in ParallelTransformIdentity::toFieldAligned). But there seems to be no way to do both with the same function signature 😞