CV - Merge Data with 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

[snlwih]

Hi Robert, and the story continues with another Computer Vision related custom step :smile:

As always here is my feedback:

• Screenshots in readme missing 1-pixel black border
• Labels for certain UI controls missing colon at end of the label
• 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 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 macro variables are generated for each of the controls on your UI (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 ou use put statements to write the log messages inside the data null step.
• Inconsistent idents, but this time only 2 lines so quick and easy to fix.

[snlwih]

@Rwinstonbnc , thanks for implementing my feedback. The code now also looks "nice", meaning consistent indenting and code patterns :smiley:

It took me some time to come to terms with using append as a join type, but I can live with that. Even though it makes the names of the input table controls confusing, as changing the "join type" potentially changes the role of the tables on each port. It also makes codegen a bit confusing. Personally I would probably have used more generic names for the input table controls (inputtable1/inputtable2) for this instead of trying to use names that reflect purpose. UX designers would probably have used a radio group to ask the user whether they wanted to perform and append or a join, and when a join was selected then show up a list of join types and ask for the name of the ID/Key field for joining.

But as the codegen is so "straightforward" I can live with this.

Still wondering why you are not using a column selector control for the ID variable? This will present the user with a selection list of columns in the input table (and I would associate the control with input port 1, the first input control in your UI definition) so they do not have to type in the name.

Will still merge the version of this step as-is.

[snlwih]

SAS Internal Due Diligence Review - COMPLETED on 04OCT2024

[Rwinstonbnc]

Hey Wilbram,

My pleasure and thank you for the guidance 😊

I understand your concerns pertaining to the table name. We just have to be careful… I’ll run some test to confirm or alleviate my concerns. Thank you for pushing the Step to Git. It may take me a few weeks to investigate so it’s good a working version will be available for use in the meantime.

I don’t believe we should use a column selector. The CV flows need to have the flexibility to be pre-built without the existence of data. I don’t believe the column selector will allow for the selection of a variable without data? Or will it? If I’m mistaken then we can consider replacing the text box with a column selector.

All the best, Robert

From: Wilbram Hazejager @.> Sent: Friday, October 4, 2024 7:42 AM To: sassoftware/sas-studio-custom-steps @.> Cc: Robert Blanchard @.>; Mention @.> Subject: Re: [sassoftware/sas-studio-custom-steps] CV - Merge Data with Images (PR #157)

@Rwinstonbnchttps://github.com/Rwinstonbnc , thanks for implementing my feedback. The code now also looks "nice", meaning consistent indenting and code patterns 😃

It took me some time to come to terms with using append as a join type, but I can live with that. Even though it makes the names of the input table controls confusing, as changing the "join type" potentially changes the role of the tables on each port. It also makes codegen a bit confusing. Personally I would probably have used more generic names for the input table controls (inputtable1/inputtable2) for this instead of trying to use names that reflect purpose. UX designers would probably have used a radio group to ask the user whether they wanted to perform and append or a join, and when a join was selected then show up a list of join types and ask for the name of the ID/Key field for joining.

But as the codegen is so "straightforward" I can live with this.

Still wondering why you are not using a column selector control for the ID variable? This will present the user with a selection list of columns in the input table (and I would associate the control with input port 1, the first input control in your UI definition) so they do not have to type in the name.

Will still merge the version of this step as-is.

— Reply to this email directly, view it on GitHubhttps://github.com/sassoftware/sas-studio-custom-steps/pull/157#issuecomment-2393874103, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHVWO6ZJU3FT7YF4HHTVIOTZZ2SM7AVCNFSM6AAAAABLBAFSVSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJTHA3TIMJQGM. You are receiving this because you were mentioned.Message ID: @.@.>>