Restore default arguments for API compatibility #78

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@roystgnr
Member
roystgnr commented Apr 8, 2013

This seems to be all the stuff Ben forgot to uncomment. I'll keep an eye out for any similar but harder to grep for issues elsewhere.

@benkirk
Member
benkirk commented Apr 8, 2013

Thanks, I got the stuff required for the examples and apps I tested, but forgot about that.

We definitely need a configure-way to disable all default communicators...

@roystgnr
Member
roystgnr commented Apr 8, 2013

Yeah, the trouble with maintaining backwards compatibility is that
when we update our code we're systematically removing everything that
would have tested backwards compatibility. The only way to fix that
might be to set up something that checks out tests from older
revisions to run them against the git head.

We definitely need a configure-way to disable all default communicators...

I'll see if I can prep the macro for this in a few days, after any
screaming has died down and any remaining bugs have been fixed from
this stage of the feature.

@friedmud
Member
friedmud commented Apr 8, 2013

I am actually not for most of these changes.

Anything that is normally behind a build() method shouldn't need a default constructor.

If you're going around the build() function to do it yourself then you should know enough to put in a Comm there.

@roystgnr
Member
roystgnr commented Apr 8, 2013

It's a question of maximizing compatibility, not foregoing an opportunity to punish ignorance. Do we gain anything by preventing people from using these methods in codes that are compatible with two successive VCS revisions of libMesh?

I would like to have a way of marking libMesh APIs as "for internal use only" eventually, but "if it breaks, you'll know it was supposed to be internal" is not that way.

@roystgnr
Member
roystgnr commented Apr 8, 2013

Many thanks for chiming in quickly, though. I was about to merge this; I'll instead hold off until there's been more discussion.

(on an unrelated note - I just sent two posts to this pull request via email "reply to all" - the first went through; the second, with the above text, got denied by "ASPMX.L.GOOGLE.com" with "550-5.1.1 The email account that you tried to reach does not exist". Odd.

@roystgnr
Member
roystgnr commented Apr 8, 2013

Speaking of breakage - would anyone mind if I remove the ParallelObject status of ErrorEstimator and the Communicator argument in its constructor? If we simply have it use the communicators available via the System or EquationSystems arguments to all that class's methods, then existing AMR app code should work fine.

@benkirk
Member
benkirk commented Apr 8, 2013

On Apr 8, 2013, at 5:47 PM, "roystgnr" <notifications@github.commailto:notifications@github.com> wrote:

Speaking of breakage - would anyone mind if I remove the ParallelObject status of ErrorEstimator and the Communicator argument in its constructor? If we simply have it use the communicators available via the System or EquationSystems arguments to all that class's methods, then existing AMR app code should work fine.

No issues but check first - I think I was forced into doing this because there were methods (summing the error vector iirc) that needed a communicator without the mesh. If you see that you might save yourself some time and leave it...

Also, this process showed to me MeshCommunication should probably be a class soon!

-Ben

@roystgnr
Member
roystgnr commented Apr 8, 2013

Yes; we'd need an ugly hack: cache a pointer to the last Communicator
seen to keep around for reduce_error() to use. And we'd still break
in the unlikely case that someone created one ErrorEstimator to use
for calculating errors but then made another one to use for summing
errors.

On the bright side, it would only be a temporary ugly hack.
Everything about the std::vector in that API is wrong: it's
hardcoded to not use ErrorVectorReal, and we need a distributed
version of ErrorVector rather than a subclass of std::vector if we
expect decent AMR/C scalability in the long term.

@benkirk
Member
benkirk commented Apr 8, 2013

Then if you are offering, sure!!

On Apr 8, 2013, at 6:02 PM, "roystgnr" <notifications@github.commailto:notifications@github.com> wrote:

Yes; we'd need an ugly hack: cache a pointer to the last Communicator
seen to keep around for reduce_error() to use. And we'd still break
in the unlikely case that someone created one ErrorEstimator to use
for calculating errors but then made another one to use for summing
errors.

On the bright side, it would only be a temporary ugly hack.
Everything about the std::vector in that API is wrong: it's
hardcoded to not use ErrorVectorReal, and we need a distributed
version of ErrorVector rather than a subclass of std::vector if we
expect decent AMR/C scalability in the long term.


Reply to this email directly or view it on GitHubhttps://github.com/libMesh/libmesh/pull/78#issuecomment-16084547.

@roystgnr
Member
roystgnr commented Apr 8, 2013

I'm definitely offering to single-handedly increase backwards compatibility for the current ErrorEstimator APIs; I'll try to have a pull request tomorrow.

I'm offering to try and help work out how to get the new ErrorVector APIs to scale properly, but it'll be a while before I find time for that and it's a significant enough bit of refactoring that I wouldn't want to do it without collective input.

Just making sure there's no ambiguity here. ;-)

@benkirk
Member
benkirk commented Apr 8, 2013

On Apr 8, 2013, at 6:24 PM, "roystgnr" notifications@github.com wrote:

Just making sure there's no ambiguity here. ;-)

Sounds fair - thanks!

@roystgnr
Member
roystgnr commented Apr 9, 2013

It looks like the best thing to do for ErrorEstimator is less of a hack.

Caching a Communicator* won't work for user-derived ErrorEstimator subclasses, because everything that might update that cache is virtual and we can't force their overrides to update the cache properly.

But if we have to break user-derived ErrorEstimator subclasses in the not-comm-world case anyway, we might as well just change ErrorEstimator::reduce_error(), since that's protected and any breakage of it will also be limited to user-derived subclasses. So I'm just going to move the optional Communicator argument to that method.

@benkirk
Member
benkirk commented Apr 9, 2013

Got tired last night -

Can we go ahead and introduce

#ifdef LIBMESH_DISABLE_DEFAULT_COMMUNICATOR_ARGUMENT
#  define LIBMESH_DEFAULT_COMMINICATOR_ARGUMENT /* = libMesh::CommWorld */
#else
#  define LIBMESH_DEFAULT_COMMINICATOR_ARGUMENT = libMesh::CommWorld
#endif

and a corresponding

$ ./configure --disable-default-communicator-argument

that may or may not be implied by --disable-default-comm-world, so that early adopters (like Derek) can continue to get compile instead of runtime errors?

By default in for the next release we leave that as an opt-in option, but quickly switch it - very much inline with the other sweeping changes (libMesh namespace, #include"libmesh/foo.h") that we have made?

@roystgnr
Member
roystgnr commented Apr 9, 2013

Let's just use the existing --disable-default-comm-world, not add an
independent option? I can't think of any reasons a user would want to
use CommWorld if they're already having to change their code to manage
non-default communicators, and they certainly wouldn't want to use a
default argument pointing to a dummy object.

I was waiting on this until after I had "make check" successful with
--disable-default-comm-world, but if anyone wants to beat me to it
that's fine.

@benkirk
Member
benkirk commented Apr 9, 2013

I'm a little overwhelmed then by the order of pull requests...

Are you close to having 'make check" successful with --disable-default-comm-world without this patch?

If so, I might wait until you commit that, and then implement the proposed option instead of this request...

@roystgnr
Member
roystgnr commented Apr 9, 2013

Getting "make check" to work shouldn't require this patch, knock on wood.

@roystgnr
Member

This was mostly mooted by the disable_default_args feature we just pulled in.

@roystgnr roystgnr closed this Apr 10, 2013
@roystgnr roystgnr deleted the roystgnr:communicator_defaults branch Apr 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment