MFlowCode/MFC

DRY m_checker

Closed this issue · 14 comments

From #544 (comment):

Take this example adapted from m_checker (there are literally hundreds of lines like this):

if (acoustic(j)%npulse >= 5 .and. acoustic(j)%dipole) then
   call s_mpi_abort('acoustic('//trim(jStr)//')%dipole is not '// &
                     'supported for support >= 5 '// &
                     '(non-planar supports). Exiting ...')
end if

This can be simplified to:

@:PROHIBIT(acoustic(j)%npulse >= 5 .and. acoustic(j)%dipole, 'for acoustic source '//trim(jStr))

This is what would get printed (I actually ran this):

m_checker.fpp:186: Prohibited condition: acoustic(j)%npulse >= 5 .and. acoustic(j)%dipole. for > acoustic source 1

I think it's much better than our current solution. Less code and less duplication. Of course you can still make this example better but I hope this proves my point.

I feel like @ChrisZYJ had already worked on an issue like this... integrating this would be a nice step up! Let me know if you want to work on it @ChrisZYJ it should be straightforward.

I don't mind doing it. It will take a while though - there are so many checkers!

True. Well you don't have to do it all in one day, up to you.

In fact, if you do all of the ones in one of the sub-codes (preprocess, sim., or postprocess), I will merge the PRs one-at-a-time.

Okay! I'll give it a try

May I know if this is all right or there's a better way?:

@:PROHIBIT(all(fd_order /= (/dflt_int, 1, 2, 4/)), '. Note: fd_order must be 1, 2, or 4')

outputs

Prohibited condition: all(fd_order /= (/dflt_int, 1, 2, 4/)). Note: fd_order must be 1, 2, or 4

Also stuff like this are giving me issues:

        if (nGlobal < 2**(min(1, m) + min(1, n) + min(1, p))*num_procs) then
            call s_int_to_str(2**(min(1, m) + min(1, n) + min(1, p))*num_procs, numStr)
            call s_mpi_abort('Total number of cells must be at least '// &
                             '(2^[number of dimensions])*num_procs, which is currently '// &
                             trim(numStr)//'. Exiting ...')
        end if

Should I keep it as it is and not change it to @:PROHIBIT?

Also stuff like this are giving me issues:

        if (nGlobal < 2**(min(1, m) + min(1, n) + min(1, p))*num_procs) then
            call s_int_to_str(2**(min(1, m) + min(1, n) + min(1, p))*num_procs, numStr)
            call s_mpi_abort('Total number of cells must be at least '// &
                             '(2^[number of dimensions])*num_procs, which is currently '// &
                             trim(numStr)//'. Exiting ...')
        end if

Should I keep it as it is and not change it to @:PROHIBIT?

what is the challenge in changing this to prohibit?

w

May I know if this is all right or there's a better way?:

@:PROHIBIT(all(fd_order /= (/dflt_int, 1, 2, 4/)), '. Note: fd_order must be 1, 2, or 4')

outputs

Prohibited condition: all(fd_order /= (/dflt_int, 1, 2, 4/)). Note: fd_order must be 1, 2, or 4

It seems like maybe this should be an ASSERT? Maybe @henryleberre has a more slick idea in mind.

Also stuff like this are giving me issues:

        if (nGlobal < 2**(min(1, m) + min(1, n) + min(1, p))*num_procs) then
            call s_int_to_str(2**(min(1, m) + min(1, n) + min(1, p))*num_procs, numStr)
            call s_mpi_abort('Total number of cells must be at least '// &
                             '(2^[number of dimensions])*num_procs, which is currently '// &
                             trim(numStr)//'. Exiting ...')
        end if

Should I keep it as it is and not change it to @:PROHIBIT?

what is the challenge in changing this to prohibit?

Maybe the first part of the "Prohibited condition: ..." message isn't as helpful as the custom message? But yeah it still works (just a bit long):

Prohibited condition: nGlobal < 2**(min(1, m) + min(1, n) + min(1, p))*num_procs. Total number of cells must be at least (2^[number of dimensions])*num_procs, which is currently 64

from the code:

        call s_int_to_str(2**(min(1, m) + min(1, n) + min(1, p))*num_procs, numStr)
        @:PROHIBIT(nGlobal < 2**(min(1, m) + min(1, n) + min(1, p))*num_procs, 'Total number of cells must be at least (2^[number of dimensions])*num_procs, which is currently '//trim(numStr))

It seems like the code you wrote is nicer than what we had before, though you might want to introduce a line break? (assuming this is allowed in macros).

Yes line breaks work in macros and I'll add them. I'll change everything to PROHIBIT then

I'm open to other ideas. Or you can concoct something with @henryleberre and bounce it off me.

Yeah I'm sure Henry will have better ideas, and I'm happy to improve the code further. Right now I'm submitting what I have as a PR draft and maybe you could check out the result and see if you like it.