Readability with CHKERRQ

[Hammond, Glenn E]

Here is an example of CHKERRQ degrading readability in the code (from RTCheckUpdatePost):

call VecGetArrayReadF90(dX,dX_p,ierr)
CHKERRQ(ierr)
call VecGetArrayReadF90(X1,X1_p,ierr)
CHKERRQ(ierr)
max_relative_change = maxval(dabs(dX_p(:)/X1_p(:)))
call VecRestoreArrayReadF90(dX,dX_p,ierr)
CHKERRQ(ierr)
call VecRestoreArrayReadF90(X1,X1_p,ierr)
CHKERRQ(ierr)
call VecGetArrayReadF90(field%flow_r,r_p,ierr)
CHKERRQ(ierr)
call VecGetArrayReadF90(field%flow_accum,accum_p,ierr)
CHKERRQ(ierr)
max_scaled_residual = maxval(dabs(r_p(:)/accum_p(:)))
call VecRestoreArrayReadF90(field%flow_r,r_p,ierr)
CHKERRQ(ierr)
call VecRestoreArrayReadF90(field%flow_accum,accum_p,ierr)
CHKERRQ(ierr)

vs.

call VecGetArrayReadF90(dX,dX_p,ierr)
call VecGetArrayReadF90(X1,X1_p,ierr)
max_relative_change = maxval(dabs(dX_p(:)/X1_p(:)))
call VecRestoreArrayReadF90(dX,dX_p,ierr)
call VecRestoreArrayReadF90(X1,X1_p,ierr)
call VecGetArrayReadF90(field%flow_r,r_p,ierr)
call VecGetArrayReadF90(field%flow_accum,accum_p,ierr)
max_scaled_residual = maxval(dabs(r_p(:)/accum_p(:)))
call VecRestoreArrayReadF90(field%flow_r,r_p,ierr)
call VecRestoreArrayReadF90(field%flow_accum,accum_p,ierr)

I am not advocating removal of CHKERRQ, but perhaps the better approach is to append in locations where it is used excessively. E.g.

call VecGetArrayReadF90(dX,dX_p,ierr); CHKERRQ(ierr)
call VecGetArrayReadF90(X1,X1_p,ierr); CHKERRQ(ierr)
max_relative_change = maxval(dabs(dX_p(:)/X1_p(:)))
call VecRestoreArrayReadF90(dX,dX_p,ierr); CHKERRQ(ierr)
call VecRestoreArrayReadF90(X1,X1_p,ierr); CHKERRQ(ierr)
call VecGetArrayReadF90(field%flow_r,r_p,ierr); CHKERRQ(ierr)
call VecGetArrayReadF90(field%flow_accum,accum_p,ierr); CHKERRQ(ierr)
max_scaled_residual = maxval(dabs(r_p(:)/accum_p(:)))
call VecRestoreArrayReadF90(field%flow_r,r_p,ierr); CHKERRQ(ierr)
call VecRestoreArrayReadF90(field%flow_accum,accum_p,ierr); CHKERRQ(ierr)

Glenn

[Jed Brown]

"Hammond, Glenn E" <geh...@sandia.gov> writes:
> I am not advocating removal of CHKERRQ, but perhaps the better approach is to append in locations where it is used excessively. E.g.
>
> call VecGetArrayReadF90(dX,dX_p,ierr); CHKERRQ(ierr)
> call VecGetArrayReadF90(X1,X1_p,ierr); CHKERRQ(ierr)

That's why we use this form in PETSc. I quickly learned to not even see
the CHKERRQ's hanging at the end of lines (except to notice when they're
missing).

[Nathan Collier]

I tried this at first, but I found that compilers complained about lines being too long (since it is a macro and replaced).

I agree that I don't like the lack of readability. I guess if I had to choose, I would rather the errors get checked. That is of course just one man's 0.02 USD.

Nate

[Nathan Collier]

I guess one alternative would be to try to detect when line lengths would be ok and insert it at the end on those lines. At least this would improve readability where possible?

Nate

[Jed Brown]

Nathan Collier <nathanie...@gmail.com> writes:

> I guess one alternative would be to try to detect when line lengths would
> be ok and insert it at the end on those lines. At least this would improve
> readability where possible?

Are there compilers for which you cannot set unlimited line length?

[Nathan Collier]

I will confess that I don't know fortran compilers that well. When I tried appending CHKERRQ, I got compile errors that looked to truncate my expression at odd places and assumed that the line limit was causing me problems.

Are you suggesting we compile with -ffree-line-length set to something large? Do we currently use the 132 default limit?

Nate

[Richard Mills]

We currently adhere to the 132 character limit so that we stay compliant with the language standard.  Actually, Glenn really prefers that we stay under 120 characters, and he is the official PFLOTRAN benevolent dictator.

--Richard

--
You received this message because you are subscribed to the Google Groups "pflotran-dev" group.

To view this discussion on the web visit https://groups.google.com/d/msgid/pflotran-dev/CAJdhdcMfzPK-gMvZsVfjVVMfApLRcUOzfcZm0%3DO9ZhRRukyM7Q%40mail.gmail.com.

[Jed Brown]

Richard Mills <r...@utk.edu> writes:

> We currently adhere to the 132 character limit so that we stay compliant
> with the language standard. Actually, Glenn really prefers that we stay
> under 120 characters, and he is the official PFLOTRAN benevolent dictator.

There is a difference between expanded macros occupying more than 120
chars and the original source doing so. The language standard doesn't
support a preprocessor at all. (The language committee doesn't seem to
care much about conveniences like error handling.)

[Richard Mills]

Yes, you are certainly right.  My brain is not firing on all cylinders this morning.  I'm guessing that once macros are expanded, we go over 132 characters pretty often.  Don't know if there are any mainstream compilers out there for which this is a problem.

--Richard

[Jed Brown]

Richard Mills <r...@utk.edu> writes:

> Yes, you are certainly right. My brain is not firing on all cylinders this
> morning. I'm guessing that once macros are expanded, we go over 132
> characters pretty often. Don't know if there are any mainstream compilers
> out there for which this is a problem.

You may need an option like -ffree-line-length-none (which PETSc finds
by default), but I'm not aware of compilers for which there is no such
option.

[Nathan Collier]

It would be easy enough to keep a 132 (or even 120) limit as the coding guide states (and we could check for compliance via a python script) and then use the option Jed proposes to allow for macro expansion. Then I could move then checks to end of line versions. Thoughts?

Nate

[Hammond, Glenn E]

I target 80 characters (not sure where the 132 characters came from).  How long is the macro in Fortran?

 

Glenn

[Jed Brown]

"Hammond, Glenn E" <geh...@sandia.gov> writes:

> I target 80 characters (not sure where the 132 characters came from). How long is the macro in Fortran?

include/finclude/petscsysdef.h:#define CHKERRQ(n) if (n .ne. 0) call MPI_Abort(PETSC_COMM_WORLD,n,n)

[Cc: petsc-dev]

Since this doesn't provide error tracing, why can't we use the ierr
argument to make the wrapper do this abort instead?

call PetscFunctionWithSeveralArgs(a,b,c,d,PetscErrAbort)

Here, PetscErrAbort would be a special value similar to
PETSC_NULL_OBJECT. If the wrapper sees it, the wrapper aborts instead
of returning. Thoughts?

[Hammond, Glenn E]

Since the macro is 50 characters in length, we are safe to append if we stick to 80 character lines in PFLOTRAN (assuming the compiler will allow 132 characters).

Glenn

[Nathan Collier]

Since the macro is 50 characters in length, we are safe to append if we stick to 80 character lines in PFLOTRAN (assuming the compiler will allow 132 characters).

A quick parse of the source code reveals that 4957 lines are wider than 80 characters. So that would be an OK solution if we fix the source. I don't think automatic multi-lining of the source is a good idea though (might be hard to make things look correct). To me this sounds like a labor intensive option that we could divide up. However, if you are serious about the 80 character limit, then we could fix the source, and then apply a check on commits to make sure you do not violate that rule in the future.

 

> Since this doesn't provide error tracing, why can't we use the ierr argument
> to make the wrapper do this abort instead?
>
> call PetscFunctionWithSeveralArgs(a,b,c,d,PetscErrAbort)
>
> Here, PetscErrAbort would be a special value similar to PETSC_NULL_OBJECT.
> If the wrapper sees it, the wrapper aborts instead of returning.  Thoughts?

Jed, I don't totally follow you here. Are you suggesting that the PETSc fortran wrapper itself could be made to call the Abort internally such that user codes do not need to do anything? If that is what you mean, then user code would have to check codes or configure with --with-errorchecking=0?

Nate

[Hammond, Glenn E]

How many of those lines are PETSc call?

 

Glenn

 

From: Nathan Collier [mailto:nathanie...@gmail.com]

Sent: Monday, July 21, 2014 1:13 PM
To: Hammond, Glenn E

[Nathan Collier]

255

Nate

[Jed Brown]

Nathan Collier <nathanie...@gmail.com> writes:
>> > Since this doesn't provide error tracing, why can't we use the ierr
>> argument
>> > to make the wrapper do this abort instead?
>> >
>> > call PetscFunctionWithSeveralArgs(a,b,c,d,PetscErrAbort)
>> >
>> > Here, PetscErrAbort would be a special value similar to
>> PETSC_NULL_OBJECT.
>> > If the wrapper sees it, the wrapper aborts instead of returning.
>> Thoughts?
>>
>
> Jed, I don't totally follow you here. Are you suggesting that the PETSc
> fortran wrapper itself could be made to call the Abort internally such that
> user codes do not need to do anything? If that is what you mean, then user
> code would have to check codes or configure with --with-errorchecking=0?

The idea is that if all a caller wants is to abort on error, they would
pass a special constant instead of 'ierr', in which case the wrapper
would abort instead of returning an error code.

[Hammond, Glenn E]

That’s doable.

[Nathan Collier]

Ok, I can work on the offending lines and moving things to the end of statement versions.

However, did you follow Jed's idea? (Step in here Jed if I missed something). The CHKERRQ(ierr) macro in fortran just calls MPIAbort if ierr is negative. He was saying that the PETSc fortran wrappers could be modified such that upon calling a PETSc function from fortran, a specific value of ierr can be passed as the final argument, say PetscErrAbort. This would signal PETSc to call Abort for you if the PETSc routine produces errors. If you hand it anything other than that, you get the output error code which you could check internally in your code. This way you can avoid ugly CHKERRQ(ierr) entirely if you want.

The other PETSc folk would have to agree but if you like the idea I can push that way instead.

Nate

--

You received this message because you are subscribed to the Google Groups "pflotran-dev" group.

To view this discussion on the web visit https://groups.google.com/d/msgid/pflotran-dev/DF0B2AE3F6166745843F1066C7E2F38E90D9E2C7%40EXMB01.srn.sandia.gov.

[Jed Brown]

Yes, that's the idea. Glenn, could you please stop dropping the
petsc-dev Cc? I'd like to hear their input.

Nathan Collier <nathanie...@gmail.com> writes:

> Ok, I can work on the offending lines and moving things to the end of
> statement versions.
>
> However, did you follow Jed's idea? (Step in here Jed if I missed
> something). The CHKERRQ(ierr) macro in fortran just calls MPIAbort if ierr
> is negative. He was saying that the PETSc fortran wrappers could be
> modified such that upon calling a PETSc function from fortran, a specific
> value of ierr can be passed as the final argument, say PetscErrAbort. This
> would signal PETSc to call Abort for you if the PETSc routine produces
> errors. If you hand it anything other than that, you get the output error
> code which you could check internally in your code. This way you can avoid
> ugly CHKERRQ(ierr) entirely if you want.
>
> The other PETSc folk would have to agree but if you like the idea I can
> push that way instead.
>
> Nate
>
>
>
>
>
>
> On Mon, Jul 21, 2014 at 4:19 PM, Hammond, Glenn E <geh...@sandia.gov>
> wrote:
>
>> That’s doable.
>>
>>
>>

>> *From:* Nathan Collier [mailto:nathanie...@gmail.com]
>> *Sent:* Monday, July 21, 2014 1:17 PM
>>
>> *To:* Hammond, Glenn E
>> *Cc:* Jed Brown; pflotr...@googlegroups.com; Richard Mills
>> *Subject:* Re: [EXTERNAL] Re: [pflotran-dev: 2196] Readability with

>> CHKERRQ
>>
>>
>>
>> 255
>>
>>
>>
>> Nate
>>
>>
>>
>> On Mon, Jul 21, 2014 at 4:14 PM, Hammond, Glenn E <geh...@sandia.gov>
>> wrote:
>>
>> How many of those lines are PETSc call?
>>
>>
>>
>> Glenn
>>
>>
>>

>> *From:* Nathan Collier [mailto:nathanie...@gmail.com]
>> *Sent:* Monday, July 21, 2014 1:13 PM
>> *To:* Hammond, Glenn E
>> *Cc:* Jed Brown; pflotr...@googlegroups.com; Richard Mills
>> *Subject:* Re: [EXTERNAL] Re: [pflotran-dev: 2196] Readability with

>> <https://groups.google.com/d/msgid/pflotran-dev/DF0B2AE3F6166745843F1066C7E2F38E90D9E2C7%40EXMB01.srn.sandia.gov?utm_medium=email&utm_source=footer>
>> .
>>

[Hammond, Glenn E]

Either way works for me.

 

Glenn

 

To view this discussion on the web visit https://groups.google.com/d/msgid/pflotran-dev/CAJdhdcNBKH8XLnraczYqCrzv%3DU%3D4WXs2jFW0NrELjAJsP1%3D-8Q%40mail.gmail.com.

[Nathan Collier]

All,

I will fix the multiline PETSc calls and change CHKERRQ to be end-of-line. This we will need anyway because we lag in the version of PETSc we support. 

If/when we add what Jed proposes to PETSc, then I can remove the checks entirely.

Nate

To view this discussion on the web visit https://groups.google.com/d/msgid/pflotran-dev/DF0B2AE3F6166745843F1066C7E2F38E90D9E34B%40EXMB01.srn.sandia.gov.