Refactored faceting logic in dlgPICSARainfall class #10044

Rockjunior · 2025-11-01T13:36:55Z

Streamlined the faceting logic by removing unused operators and consolidating facet parameters. This enhances code clarity and maintainability.

gemini-code-assist · 2025-11-01T13:37:12Z

Summary of Changes

Hello @Rockjunior, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on refactoring the faceting logic within the dlgPICSARainfall class to improve code clarity and maintainability. It achieves this by eliminating obsolete ROperator instances and introducing a new RFunction, clsVarsFunction, to centralize the management of facet variables. The changes streamline how facet parameters are constructed and applied, making the code more efficient and easier to understand.

Highlights

Removed Unused Operators: The clsFacetRowOp and clsFacetColOp ROperator instances, along with their associated logic and parameter handling, have been entirely removed or commented out as they were no longer necessary for the faceting process.
Introduced clsVarsFunction: A new clsVarsFunction RFunction has been added and integrated to manage variable parameters for faceting, providing a more streamlined and consolidated approach to defining facet variables.
Refactored Faceting Logic: The core logic for constructing and applying facet parameters to clsFacetFunction has been significantly refactored. It now explicitly handles 'facets', 'rows', and 'cols' parameters using the new clsVarsFunction, enhancing clarity and maintainability.
Parameter Cleanup: Additional RemoveParameterByName calls have been introduced for clsFacetFunction to ensure proper cleanup of 'facets', 'rows', 'ggplot2::rows', and 'cols' parameters, preventing potential state issues.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

Rockjunior · 2025-11-01T13:38:58Z

Fixes #10035
@berylwaswa @lilyclements @Vitalis95 Kindly review

gemini-code-assist

Code Review

This pull request refactors the faceting logic, which is a good step towards improving code clarity. However, a significant amount of old code has been commented out. It is better to remove dead code to improve readability and maintainability, relying on version control to access historical code. I've left several comments suggesting the removal of this commented-out code. Additionally, I found a high-severity bug in the AddRemoveGroupBy method where the grouping for facet_grid is missing, which would lead to incorrect plots. I've provided a suggestion to fix this issue.

gemini-code-assist · 2025-11-01T13:39:24Z

instat/dlgPICSARainfall.vb

                Select Case ucrInputStation.GetText()
                    Case strFacetWrap
                        GetParameterValue(clsFacetOperator)
-                    Case strFacetCol, strFacetColAll
-                        GetParameterValue(clsFacetColOp)
-                    Case strFacetRow, strFacetRowAll
-                        GetParameterValue(clsFacetRowOp)
+                        'Case strFacetCol, strFacetColAll
+                        '    GetParameterValue(clsVarsFunction)
+                        'Case strFacetRow, strFacetRowAll
+                        '    GetParameterValue(clsFacetRowOp)
                End Select


The grouping logic for facet_grid seems to be missing after the refactoring. When using facet_grid (i.e., for strFacetCol or strFacetRow), the faceting variable should be added to clsGroupByFunction to ensure that summary statistics are calculated per facet. Without this, summaries like mean lines will be calculated globally across all facets, leading to incorrect plots. The commented-out code suggests this was intended but incomplete, possibly due to a type mismatch with GetParameterValue.

Select Case ucrInputStation.GetText() Case strFacetWrap GetParameterValue(clsFacetOperator) Case strFacetCol, strFacetColAll, strFacetRow, strFacetRowAll If Not ucrReceiverFacetBy.IsEmpty Then clsGroupByFunction.AddParameter(clsGroupByFunction.iParameterCount, ucrReceiverFacetBy.GetVariableNames(bWithQuotes:=False), bIncludeArgumentName:=False) End If End Select

gemini-code-assist · 2025-11-01T13:39:24Z

instat/dlgPICSARainfall.vb

+    'Private clsFacetRowOp As New ROperator
+    'Private clsFacetColOp As New ROperator


These variable declarations are commented out. To improve code clarity and maintainability, it's better to remove dead code instead of commenting it out. Version control can be used to track and restore old code if needed.

gemini-code-assist · 2025-11-01T13:39:24Z

instat/dlgPICSARainfall.vb

+        'clsFacetRowOp = New ROperator
+        'clsFacetColOp = New ROperator


These object initializations are commented out. Please remove this dead code to improve maintainability.

gemini-code-assist · 2025-11-01T13:39:24Z

instat/dlgPICSARainfall.vb

+        'clsFacetRowOp.SetOperation("+")
+        'clsFacetRowOp.bBrackets = False
+        'clsFacetColOp.SetOperation("+")
+        'clsFacetColOp.bBrackets = False


This block of code is commented out. It's preferable to remove unused code rather than commenting it out to keep the codebase clean.

gemini-code-assist · 2025-11-01T13:39:24Z

instat/dlgPICSARainfall.vb

        clsFacetOperator.bForceIncludeOperation = True
        clsFacetOperator.bBrackets = False
-        clsFacetFunction.AddParameter("facets", clsROperatorParameter:=clsFacetOperator, iPosition:=0)
+        'clsFacetFunction.AddParameter("facets", clsROperatorParameter:=clsFacetOperator, iPosition:=0)


This line is commented out. Please remove it to improve readability.

gemini-code-assist · 2025-11-01T13:39:24Z

instat/dlgPICSARainfall.vb

+        'clsFacetColOp.RemoveParameterByName("col" & ucrInputStation.Name)
+        'clsFacetRowOp.RemoveParameterByName("row" & ucrInputStation.Name)


These lines are commented out. Please remove them to improve code readability.

gemini-code-assist · 2025-11-01T13:39:24Z

instat/dlgPICSARainfall.vb

            Case strFacetCol, strFacetColAll
                ucrReceiverFacetBy.ChangeParameterName("col" & ucrInputStation.Name)
-                ucrReceiverFacetBy.SetRCode(clsFacetColOp)
+                'ucrReceiverFacetBy.SetRCode(clsFacetColOp)


This line is commented out. Please remove it.

gemini-code-assist · 2025-11-01T13:39:24Z

instat/dlgPICSARainfall.vb

            Case strFacetRow, strFacetRowAll
                ucrReceiverFacetBy.ChangeParameterName("row" & ucrInputStation.Name)
-                ucrReceiverFacetBy.SetRCode(clsFacetRowOp)
+                'ucrReceiverFacetBy.SetRCode(clsFacetRowOp)


This line is commented out. Please remove it.

gemini-code-assist · 2025-11-01T13:39:24Z

instat/dlgPICSARainfall.vb

+        'If bRow OrElse bRowAll Then
+        '    'clsFacetOperator.AddParameter("left", clsROperatorParameter:=clsFacetRowOp, iPosition:=0)
+        'ElseIf (bCol OrElse bColAll) AndAlso bWrap = False Then
+        '    clsFacetOperator.AddParameter("left", ".", iPosition:=0)
+        'Else
+        '    clsFacetOperator.RemoveParameterByName("left")
+        'End If
+
+        'If bCol OrElse bColAll Then
+        '    clsFacetOperator.AddParameter("right", clsROperatorParameter:=clsFacetColOp, iPosition:=1)
+        'ElseIf (bRow OrElse bRowAll) AndAlso bWrap = False Then
+        '    clsFacetOperator.AddParameter("right", ".", iPosition:=1)
+        'Else
+        '    clsFacetOperator.RemoveParameterByName("right")
+        'End If


This large block of code is commented out. It seems to be part of the old logic that has been refactored. Please remove this dead code to improve maintainability.

lilyclements

@anastasia-mbithe can you review this? It is to check that the plot works now for the PICSA rainfall graph when teh facet option is row, column, row + o, and column + o

lilyclements

Looks good! Only issue I'm finding is when "none" is selected. At the moment it runs ggplot2::facet_wrap(), but actually we want to run nothing. We want to remove that function entirely, rather than the parameter value.

…d parameters and simplifying facet handling

Rockjunior · 2025-11-01T16:12:38Z

Looks good! Only issue I'm finding is when "none" is selected. At the moment it runs ggplot2::facet_wrap(), but actually we want to run nothing. We want to remove that function entirely, rather than the parameter value.

Fixed Now

lilyclements

@Rockjunior nice! Works well and looks good to me. Good stuff.
@anastasia-mbithe I believe you were reviewing earlier. Are you happy or do you have any comments?

anastasia-mbithe

This looks good.

Refactor faceting logic in dlgPICSARainfall class

e23f29d

gemini-code-assist bot reviewed Nov 1, 2025

View reviewed changes

Rockjunior mentioned this pull request Nov 1, 2025

PICSA Rainfall Dialog: Update facet_grid argument #10035

Open

Trigger compile check

c49f5f3

lilyclements reviewed Nov 1, 2025

View reviewed changes

lilyclements added 2 commits November 1, 2025 18:06

fix in r parameter

267d950

minor fix in add/remove facets sub

24b39b2

lilyclements reviewed Nov 1, 2025

View reviewed changes

Refactored faceting logic in dlgPICSARainfall class by removing unuse…

882d877

…d parameters and simplifying facet handling

lilyclements previously approved these changes Nov 1, 2025

View reviewed changes

lilyclements mentioned this pull request Nov 1, 2025

Update facet_grid arguments #10031

Open

Clean up extra blank lines in dlgPICSARainfall.vb.

0535877

Rockjunior dismissed lilyclements’s stale review via 0535877 November 2, 2025 06:33

anastasia-mbithe approved these changes Nov 2, 2025

View reviewed changes

lilyclements merged commit a948333 into IDEMSInternational:master Nov 2, 2025
3 checks passed

		'Private clsFacetRowOp As New ROperator
		'Private clsFacetColOp As New ROperator

		'clsFacetRowOp = New ROperator
		'clsFacetColOp = New ROperator

		'clsFacetColOp.RemoveParameterByName("col" & ucrInputStation.Name)
		'clsFacetRowOp.RemoveParameterByName("row" & ucrInputStation.Name)

Refactored faceting logic in dlgPICSARainfall class #10044

Refactored faceting logic in dlgPICSARainfall class #10044

Conversation

Rockjunior commented Nov 1, 2025

Uh oh!

gemini-code-assist bot commented Nov 1, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

Rockjunior commented Nov 1, 2025 • edited by lilyclements Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

lilyclements left a comment

Choose a reason for hiding this comment

Uh oh!

lilyclements left a comment

Choose a reason for hiding this comment

Uh oh!

Rockjunior commented Nov 1, 2025

Uh oh!

lilyclements left a comment

Choose a reason for hiding this comment

Uh oh!

anastasia-mbithe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Rockjunior commented Nov 1, 2025 •

edited by lilyclements

Loading