5
\$\begingroup\$

I've started to write my first APL project, so I would like to have general feedbacks on coding style, file organization, error handling and defensive programming, performance improvements, etc.

The PNM format is handled by the following two functions:

    ∇img←readpnm file;data;x;y;type;⎕IO
     ⎕IO←1
     data←83 ¯1 ⎕MAP file
     type←(3 2⍴'P4P5P6')∧.=⎕UCS 2↑data
     →(~∨/type)⍴err
     data←3↓data
     x y←⌽⍎⎕UCS ¯1↓(⍳∘10↑⊢)data
     →type/pbm pgm ppm
    pbm:
     data←,⍉(8⍴2)⊤256|(⍳∘10↓⊢)data
     img←y(↑⍤1)(x,8×⌈8÷⍨y)⍴data
     →0
    pgm:
     img←x y⍴256|(⍳∘10↓⊢)⍣2⊢data
     →0
    ppm:
     img←(1⌽⍳3)⍉x y 3⍴256|(⍳∘10↓⊢)⍣2⊢data
     →0
     err:'Unknown file magic'
    ∇

    ∇img writepnm(spec file);hdr;dta;x;y;tie
     →((3 2⍴'P4P5P6')∧.=spec)/pbm pgm ppm
     'Unknown file kind'
     →0
    pbm:
     hdr←10,⍨⎕UCS(⍕x y←⌽⍴img)
     dta←,⍉(8×⌈8÷⍨x)↑⍉img
     dta←2⊥⍉((8÷⍨≢dta),8)⍴dta
     →fin
    pgm:
     hdr←10 50 53 53 10,⍨⎕UCS(⍕⌽⍴img)
     dta←,img
     →fin
    ppm:
     hdr←10 50 53 53 10,⍨⎕UCS(⍕x y←⌽1↓⍴img)
     dta←,y(3×x)⍴(¯1⌽⍳3)⍉img
    fin:
     tie←file(⎕NCREATE⍠'IfExists' 'Error')0
     :Trap 0
         tie ⎕ARBOUT(⎕UCS spec),10,hdr,dta
     :Else
         'Error occured when write file'
     :EndTrap
     ⎕NUNTIE tie
    ∇

The namespace Bluenoise contains a mkda function that generates a ,⍨2×r shape "dither array" suitable for halftoning using functions in Dither namespace. Their usage summaries are dither_array diffuse greyscale_image(error diffusion halftoning), dither_array dither greyscale_image dither_array (R G B colordither) greyscale_image where R G B is a size 3 vector corresponding to the weight of each color channel.

The Blur namespace has radius boxblur image, (level radius) gblur image, where gblur is approximate Gaussian blur.

The Interpol namespace has image nearest newsize, which computes nearest neighbor interpolation.

The project is at https://github.com/LdBeth/pnmaster

:Namespace Image

    tiling←⊣⍴⊢/⍤⊣⍴⍤1⊢
    quant←{⊃∘⍋⍤1⊢|⍵∘.-⍺}
    linear←{(1÷⍵-1)×⎕IO-⍨⍳⍵}

    :Namespace Bluenoise
        ∇da←mkda r
         ;m;g;s;gauss;cluster;void;imax
         ;l;v;bp;pt;rank;loc;all;⎕IO
         ⎕IO←0
         m←,⍨2×r ⋄ bp←?m⍴2
         gauss←{*-4.5÷⍨∘.+⍨2*⍨(⌽,1∘↓)⍳1+⍵}
         ⍝ use interger approximation
         s←⍴g←s⌿(s←0≠+/g)/g←⌊0.5+(⊢××/∘⍴)gauss r
         cluster←⊢×(-,⍨r)↓(,⍨r)↓{+/,g×⍵}⌺s∘(r⊖⍪⍨)∘(r⌽,⍨)
         void←cluster~ ⋄ imax←,⍳(⌈/⌈/)
         loop:
         l←imax cluster bp
         (l⌷,bp)←0
         v←imax void bp
         (v⌷,bp)←1
         →(l≠v)/loop
         pt←bp ⋄ da←m⍴0 ⋄ rank←¯1++/,bp
         :While rank≥0
             loc←imax cluster pt
             (loc⌷,pt)←0
             (loc⌷,da)←rank
             rank-←1
         :EndWhile
         pt←bp ⋄ rank←+/,bp ⋄ all←×/m
         :While rank<all
             loc←imax void pt
             (loc⌷,pt)←1
             (loc⌷,da)←rank
             rank+←1
         :EndWhile
        ∇
    :EndNamespace

    :Namespace Dither
        diffuse←{
            ⎕DIV←⎕IO←1
            in←1-⍵÷255
            g←1+∘.+⍨0 1 0
            m←⍺ ##.tiling⍨⍴⍵
            cvol←{+/,g×⍵}⌺3 3
            ⊃{
                b←0.5≤a←in×m=⍺
                err←a-b
                c←err÷cvol m>⍺
                in+←cvol c
                b∨⍵}/(⌽⍳≢,⍺),0
        }
        dither←{
            (1-⍵÷255)≥(⍴⍵)##.tiling(0.5∘+÷≢∘,)⍺
        }
        colordither←{
            q←{⌊r×⌊(r←255÷⍺-1)÷⍨⍵}
            s←⍵+(255÷(0.5+≢,⍺)×⍺⍺-1)×⍤0 2⊢(⍴⍵)##.tiling ⍺
            255⌊⍺⍺(q⍤0 2)s
        }
    :EndNamespace

    :Namespace Blur
        boxblur←{⌊⍵÷⍥({+/,⍵}⌺(1+2×⍺ ⍺))1⍴⍨⍴⍵}
        gblur←{
            ⎕IO←0
            l rad←⍺
            m←(⍴⍵)⍴1
            box←{⍵÷⍥({+/,⍵}⌺(⍺ ⍺))m}
            bforg←{
                wl←(⊢-(~2∘|))⌊0.5*⍨1+⍺÷⍨i←12×⍵*2
                m←(⌊0.5+⊢)(¯4×1+wl)÷⍨i-+/(wl*2 1 0)×1 4 3×⍺
                wl+2×m≤⍳⍺
            }
            ⌊⊃box/(l bforg rad),⊂⍵
        }
    :EndNamespace

    :Namespace Interpol
        nearest←{
            ⍝ img←⍺ ⋄ x y←⍵
            ⍺⌷⍨(⍴⍺)(##.quant⍥##.linear)¨⍵
        }
    :EndNamespace
:EndNamespace
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Coding style

Comment your code

My father brought be up with the principle that you should be able to reconstruct your code from your comments. Maybe that level of commenting isn't necessary, especially if you use well-chosen names, but at least comment your API functions with what they do, what the arguments are, and what the results are.

Adopt a naming convention

You make use of constants, variables, functions, and operators, but without any discernible naming convention other than namespaces being uppercased and everything else lowercased. Furthermore, multi-word names and abbreviations in names are all run together. This makes it hard for the human reader to parse your code and remember calling conventions. Consider adopting or adapting a naming convention and separating words and abbreviations with under_scores or camelCase. As an example, take my personal naming conventions.

Explain your names

Many of your names are very short. Consider adding a comment when such names are first used, explaining their mnemonic, as this will help the reader of your code.

Avoid excessive use of to swap arguments

I'm a big proponent of using to avoid parentheses, however, if using leads to the left argument needing a parenthesis then there is nothing to be gained. For example, (r←255÷⍺-1)÷⍨⍵ can become ⍵÷r←255÷⍺-1 and (¯4×1+wl)÷⍨i-+/(wl*2 1 0)×1 4 3×⍺ can be (i-+/(wl*2 1 0)×1 4 3×⍺)÷¯4×1+wl.

Use to simplify code

Whether to use in simple cases or not, is a matter of style. I personally do it, and I see you have too e.g. with ⍺ ##.tiling⍨⍴⍵. However, I'd be consistent then and also do it with e.g. (x,8×⌈8÷⍨y)⍴data as data⍴⍨x,8×⌈8÷⍨y and (⍴⍵)##.tiling ⍺ as ⍺ ##.tiling⍨ ⍴⍵ and (⍴⍵)⍴1 as 1⍴⍨⍴⍵ although this latter example could also be written as 1⍨¨⍵, utilising the "Constant" meaning of .

Remove unnecessary parentheses

Besides for governing order of execution and binding, parentheses can clarify structure in the code. APL's very simple precedence rules means that there's no need for parentheses "just to be sure". Even in the absence of a naming convention, primitives have a clear syntactic role, and thus things like (##.quant⍥##.linear)¨ can be ##.quant⍥##.linear¨. Alternatively, you could "factor out" the dotting into the parent space as ##.(quant⍥linear¨) or ##.(quant⍥linear)¨.

There's no need for any parenthesis in ⎕UCS(⍕⌽⍴img) and ⎕UCS(⍕x y←⌽1↓⍴img).

Neither is there in {+/,⍵}⌺(⍺ ⍺) as stranding binds stronger than almost anything else.

Avoid old-school use of

Your uses of can and should be replaced by proper control structures for clarity and to avoid coding errors. E.g. loop:→(l≠v)/loop becomes :Repeat:Until l=v and

     →type/pbm pgm ppm
    pbm:
     data←,⍉(8⍴2)⊤256|(⍳∘10↓⊢)data
     img←y(↑⍤1)(x,8×⌈8÷⍨y)⍴data
     →0
    pgm:
     img←x y⍴256|(⍳∘10↓⊢)⍣2⊢data
     →0
    ppm:
     img←(1⌽⍳3)⍉x y 3⍴256|(⍳∘10↓⊢)⍣2⊢data
     →0

becomes

     :Select type⍳1
     :Case 1 ⍝ pbm
         data←,⍉(8⍴2)⊤256|(⍳∘10↓⊢)data
         img←y(↑⍤1)(x,8×⌈8÷⍨y)⍴data
     :Case 2 ⍝ pgm
         img←x y⍴256|(⍳∘10↓⊢)⍣2⊢data
     :Case 3 ⍝ ppm
         img←(1⌽⍳3)⍉x y 3⍴256|(⍳∘10↓⊢)⍣2⊢data
     :EndSelect

Similarly in writepnm.

Be consistent in ⎕IO usage

Some of your functions use ⎕IO explicitly to be ⎕IO-independent, some localise ⎕IO←0 and some ⎕IO←1. Your code would be easier to follow if you settled on a specific value and set it once in the outermost namespace.

Alternatively, use one main value and set the other locally when needed. With this usage, the main value is usually 1 and the local 0.

Avoid inline anonymous multi-line functions

The inner multi-line dfn in diffuse is being used in exactly the same manner as bforg in gblur, yet isn't named and called separately:

            ⊃{
                b←0.5≤a←in×m=⍺
                err←a-b
                c←err÷cvol m>⍺
                in+←cvol c
                b∨⍵}/(⌽⍳≢,⍺),0

Using inline dfns like this is not only hard to read, it is also very confusing to trace through and debug because the flow is in the following order:

            7{
                2
                3
                4
                5
                6  }/1

Give the function a proper name and use that.

Parenthesise multiple assignment

Dyalog recommends that the names (…) are enclosed in parentheses to reduce potential ambiguity in assignment statements. [source]

Thus, l rad← should be written as (l rad)← and x y← should be (x y)←

Reverse order of variable names instead of reversing the data

x y←⌽ can be simplified to y x←.

Avoid unnecessary trains

Sometimes a plain explicit expression does the trick: (⌊0.5+⊢) can simply be ⌊0.5+ which will also run faster.

Functions should return a result

Though no result is needed from writepnm, it is still good practice to return a result. If you do not return a result, it is very awkward to use the function from inside dfns, or inline in expressions including trains. To prevent cluttering when used in an APL session, you can make the result shy by putting the result in braces in the function header: {result}←img writepnm(spec file);hdr;dta;x;y;tie. A possible sensible result could be the number of bytes written or 1 to indicate that all went well.

Use ⎕NAPPEND rather than ⎕ARBOUT

⎕NAPPEND is the normal way to write to binary files.

Unnecessary reshaping

As far as I can tell, the reshaping in ,y(3×x)⍴ is a no-op since the data is ravelled immediately afterwards.

Avoid reusing variable names for unrelated values

In the single expression s←⍴g←s⌿(s←0≠+/g)/g←⌊0.5+(⊢××/∘⍴)gauss r the variable name s is used for two unrelated values. This is bound to confuse the casual reader.

Make sure code is restartable

The code s←⍴g←s⌿(s←0≠+/g)/g←⌊0.5+(⊢××/∘⍴)gauss r changes the value of g and s twice. This means that if something goes wrong in this line, and you have to re-evaluate it, g and/or s may already have gone through their first transformation, and you'll have to back up until their initial assignment. Instead, consider breaking the expression into three lines, which also becomes much easier to read:

g←⌊0.5+(⊢××/∘⍴)gauss r
s←0≠+/g
s←⍴g←s⌿s/g

In fact, the refactoring might inspire you to make the filtering of g in-place:

g←⌊0.5+(⊢××/∘⍴)gauss r
s←0≠+/g
g/⍨←s ⋄ g⌿⍨←s
s←⍴g

Use for more elegant code

imax←,⍳(⌈/⌈/) can be written as imax←,⍳⌈/⍣2 which hints at the rank too.

Use to simplify tacit functions

Thus ⊢-(~2∘|) can be written as ⊢-∘~2∘|.

File organization

You've organised all code into two scripted namespaces, which means that changing any code affects the source file for a lot of other code. Also, the casing of your file names does not match the casing of their contents.

Instead, use Link and make every namespace and sub-namespace represented by a folder and subfolder, of matching name, in your repo. You can either choose to keep leaf namespaces scripted, or even break those up with one function/operator in each file.

If you unscript everything your only source files will be .apln for functions and .aplo for operators. However, you will have to wrap tacit functions in tradfns covers, e.g.:

tiling←tiling
tiling←⊣⍴⊢/⍤⊣⍴⍤1⊢

Either scheme will allow you to edit and track changes on a much more granular basis, will allow you to use file handling to move items around, and will allow you to edit multiple items simultaneously.

Error handling and defensive programming

Raise proper errors rather than returning messages

Your PNM file handling functions return character vector messages, which are non-conforming results, rather than signalling proper errors when something goes wrong. This would make for rather awkward usage, as one has to check the return value rather than simply trapping errors. Read up on error trapping with Dyalog APL (note the list of external links at the bottom).

Don't forget to untie the file if an error happens!

Assert valid input

readpnm goes right ahead and attempts a ⎕MAP without checking that the input is even a character vector or that the file exists. If fed an invalid array or even a filename for a file that doesn't exist, the user will see readpnm suspend into the tracer. Consider wrapping the code body in:

:If 1≥|≡file ⍝ vector
:AndIf 0 2∊⍨10|⎕DR file ⍝ character
:AndIf ⎕NEXISTS file

 ⋮

:Else
    ⎕SIGNAL⊂('EN' 11)('Message' 'Invalid file name')
:EndIf

Similar tests can be done for other API functions.

Avoid unnecessary option setting

⎕NCREATE will error if you try to create a file that already exists, so there's no need to use variant in (⎕NCREATE⍠'IfExists' 'Error').

Avoid dangerous usage of

The code x y←⌽⍎ blindly executes part of the file contents, which should generally be avoided. Consider either filtering the file contents or using the safe ⎕VFI: y x←2↑⊃⊢⍤//⎕VFI (The 2↑ here is to ensure we get exactly two dimensions and thus avoid a length error upon assignment.)

Performance improvements

Use built-in type conversion rather than implementing it

As far as I can tell, ,⍉(8⍴2)⊤256| converts signed 1-byte integers to signed 1-byte integers, and then further to individual bits. You can probably speed things up by only modifying the internal type (which doesn't actually change the data bits) rather than doing the computation: 11⎕DR⊃0 83⎕DR. Note the 0 83⎕DR which ensures the numbers are interpreted as unsigned integers even if they have been squeezed to Boolean (when all numbers are zeros and ones).

Use in-place changes to avoid memory copying

When reading a PBM file, (x,8×⌈8÷⍨y)⍴data reshapes data for creating img but data is never used again. Since the interpreter doesn't know that data won't be used, it has to keep its value available, and thus the reshaping requires copying the entire data in memory. If instead you do a modified assignment to perform the reshaping, it can be done in-place: data⍴⍨←x,8×⌈8÷⍨y

Similarly for PGM and PPM files, you might be able to avoid a memory copy by computing the amount of data to drop and then dropping it in-place: data↓⍨←2⍳⍨+\10=data – however, be aware that this traverses the data in its entirety to finding all the 10 before it can identify the second 10, so it might actually suffer in performance. Morale: Run some speed tests.

Avoid grade for finding the position of the smallest element

quant uses ⊃∘⍋⍤1 to find the position of the smallest element in each row. Depending on your Dyalog version, you may find (⊢⍳⌊/)⍤1 to be significantly faster.

Consider computing constants once

The constant g←1+∘.+⍨0 1 0 will be recomputed every time diffuse is used. Consider defining it once in the namespace, or at least simplifying its definition to g←3 3⍴1 2 1 2 3 2 1 2 1.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Adám, this is interesting to read. I had heard of APL, but not seen any longer bits of code until I ran into this question yesterday. Usually I can give comments on inefficient implementation of image processing functionality, even if written in a language I’ve never used. But this code really stumped me. I spent some time yesterday reading up on APL, and am quite curious about it now. I recognize some of the operators now, but a till, I think it would take a long time to read (just like a mathematical expression you haven’t seen before can take some time to parse and understand). (1/2) \$\endgroup\$ Sep 13, 2021 at 1:29
  • 1
    \$\begingroup\$ Is my impression right, or do you eventually get used to language enough to be able to read it quickly like other languages? I did read a comment that it’s a “write-only language”, which would be surprising because nobody would use it, or? \$\endgroup\$ Sep 13, 2021 at 1:31
  • \$\begingroup\$ @CrisLuengo It's almost like learning any other language. I'd say APL is one of the rare ones that actually tries to make its own style when most languages borrow from ALGOL's style or C. This make it stand out to most programmers, and when something looks unfamiliar, programmers have a bad habit of shunning it and calling it "write-only". \$\endgroup\$
    – Razetime
    Sep 13, 2021 at 2:40
  • \$\begingroup\$ @CrisLuengo If you'd like some more help on understanding APL, The APL Orchard(SE chat) is open to all. \$\endgroup\$
    – Razetime
    Sep 13, 2021 at 2:42
  • \$\begingroup\$ ⊢⍤//⎕VFI it seems you mean to take the second for the result from ⎕VFI, but in this way the result is boxed so it doesn't make sense to 2↑, I suggest using ⊃⌽⎕VFI. \$\endgroup\$
    – LdBeth
    Sep 13, 2021 at 5:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.