CV - Load Images

[Rwinstonbnc]

trafficstars

Please include answers to these questions as part of your pull request

In the GitHub webUI, use the Write tab to modify the Markdown text that is part of the pull request. For each question simply place an X inside the square brackets, [X], that represents your answer. Make sure there are no blanks inside the brackets, otherwise MarkDown doesn't render properly. Using the Preview tab while editing this form, you can see the formatted/rendered version of the message.

• Q1: Confirm that you have the right to submit the code that is being contributed. Please consider the origin of your code and confirm you have the appropriate rights to make the submission subject to the Apache 2.0 license that applies to everything in this repository of custom steps. If so, follow the instructions for the Contributor Agreement (which is based on the industry-standard Developer Certificate of Origin (DCO)).
  • [X] Yes, I have the right to submit the contributed code on behalf of myself, my company, or any other owner of the code. I have also attached my signed copy of the DCO to this message.
  • [ ] No
• Q2: Confirm that your contribution does not include any personally identifiable information (PII), for example, in any examples used in your README file.
  • [X] My contribution does NOT include PII data
  • [ ] My contribution includes PII data
• Q3: Confirm your contribution does not include any encryption or other export-controlled technology.
  • [X] My contribution does NOT contain encryption or other export-controlled technology
  • [ ] My contribution includes encryption or other export-controlled technology

ContributorAgreement.txt

[SundareshSankaran]

ContributorAgreement.txt

[Rwinstonbnc]

Here is Sundaresh's contributor agreement ContributorAgreement.txt

[snlwih]

Hi Robert, thanks for another custom step in the Computer Vision (CV) category.

Here is my feedback

• name of your custom step seems to include a blank between "images" and ".step". Not sure if/how that impacts behaviour, but just to be sure can you remove that blank?

• screenshots for tabs in your custom step should preferably have a 1-pixel black outline

• cosmetics: the line with equals signs in the About tab is longer than the title of the step.
  

• Could you use section controls for each of the logical sections in your About box (pre-reqs / doc / .parameters / input / output) and probably have most of them collapsed by default. Just like is done in the _template.step? Makes the About box so much easier to read, and also causes the user not to be "overwhelmed" when they open the About tab for the first time. The only parts that should always be visible and therefore not inside sections are the general introduction section at the top and the Authors/History section at the bottom.

• Make sure you update the version date when you make those changes. This is a common oversight when contributors implement feedback ;-)

• And then some feedback that I already provided while reviewing your other contributions earlier this week:

  • Codegen that is part of the SAS Studio Custom Step framework provides dedicated macro variables for libref and table (and more). So no need to write extract that info the name of an inputtable or outputtable control, simply use inputtable_lib, ... When you look at the generated code when you use your custom step and select "Unfold All Regions" in the generated code you can see what is generated (or read the SAS Studio documentation ;-))

  • You should not assume that for a libref that points to a caslib the name of libref and caslib are the same. I've provided code snippets that explain how to retrieve that info from the dictionary tables in comments for your previous contributions.

  • Use consistent coding for writing errors to the SAS log and stopping further execution. Sometimes you write the log messages using %put, followed by a data null step with just an abort statement. Other times use put statements to write the log messages inside the data null step.

  • Inconsistent indents, here is an example:

  • Specify the macro name on your %mend statements

  • Code lines 200-240 seem to have two data steps and neither of them are finished with a run;. ALthough that works because SAS recogizes step boundaries, it does not reflect recommended coding practices.

  • Btw. in that same code section you might want to consider adding an empty line just before the %end, as to visually separate the "end;" belonging to an if/then/do inside the data step, from the "%end;" belonging to an %if/%then/%do at the SAS macro level. My thinking is that it makes it easier on the brain to detect the differences, but perhaps it's just me.

[snlwih]

This step also generates a warning about macros being deleted that do not exist: WARNING: Extraneous argument text on %SYSMACDELETE call ignored: CHKVISUALIZEIMAGES CHECK_FOR_CASLIB

If some macros are only generated conditionally. Here is a code snippet that is part of _template.step ath deletes SAS macros and SAS macro variables conditionally:

%if %symexist(_myMacroVariable)=1 %then %do; 
   %symdel _myMacroVariable;
%end;

%if %sysmacexist(_doIt)=1 %then %do; 
   %sysmacdelete _doIt;
%end;

[snlwih]

@Rwinstonbnc , I've seen that you implemented a large part of my feedback. Thanks for that. However, I still see a number of issues that I listed earlier that should be very straightforward to fix and improve readability of your code.

• Data steps are not ended with a run; statement. So when looking at your code the reader keeps wondering whether the macro statements that are following the data step will generate more statements that will be executed in that data step, making the code unnecessarily more difficult to udnerstand. Adding a run; statement provides clarity. Sure, the code will work without such a run; statement, but we consider it bad practice. Sorry. Talking about the data step in line 153-169 and the data step in line 174-109.
• Last updated dates are out of sync between readme and About tab. Could you fix this (and have both of them represent the last date you changed anything in the step contribution)?
• The inconsistent indents as shown in the screenshot from my 24JUL comment are still there?
• Inconsistent syntax used for CAS action invocation, sometimes you use the action keyword other times not. Both are of course allowed and supported, but would it be possible to only use one pattern and use that consistently?

[snlwih]

Forgot to mention the following: I also noticed that your code performs a loadactionset call. In SAS code this is not needed, when you call an action using fully-qualified action names as in actionset.actionname, then proc cas will check if the actionset is already loaded and if not load that for you automatically. I know this syntax is needed when writing Python, but using SAS code there is no need for this. Not that important, but I wanted to make sure you knew about this.

[Rwinstonbnc]

Hey Wilbram,

Thank you for the feedback. I have hopefully corrected these issues. I'm concerned about the formatting. I'm using SAS's auto formatting tool in SAS Studio so I hope that will suffice. I thought I used it last time but perhaps my oversight of the missing run; may have been the culprit of the bad format.

All the best, Robert

<...removed unnecessary text clutter originating from using email instead of GitHub webUI to respond to comments...>

[snlwih]

The "end of the tunnel is in sight" :-) Agreed that format code in SAS Studio probably could do a better job for proc cas action calls, but at least it indents the list of action parameters that typically cover multiple code lines.

Hope that you agree that the code now looks more consistently formatted, which is a contributor to "ease of understanding and (perceived) quality" :wink:

Will merge this latest version.

[snlwih]

SAS Internal Due Diligence Review - COMPLETED on 09OCT2024