Skip to content

Refactored faceting logic in dlgPICSARainfall class #10044

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 2, 2025
Merged

Refactored faceting logic in dlgPICSARainfall class #10044

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e23f29d
Refactor faceting logic in dlgPICSARainfall class
Rockjunior Nov 1, 2025
c49f5f3
Trigger compile check
lilyclements Nov 1, 2025
267d950
fix in r parameter
lilyclements Nov 1, 2025
24b39b2
minor fix in add/remove facets sub
lilyclements Nov 1, 2025
882d877
Refactored faceting logic in dlgPICSARainfall class by removing unuse…
Rockjunior Nov 1, 2025
0535877
Clean up extra blank lines in dlgPICSARainfall.vb.
Rockjunior Nov 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 51 additions & 41 deletions instat/dlgPICSARainfall.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ Public Class dlgPICSARainfall
Private clsCLimitsYDate As New RFunction
Private clsFacetFunction As New RFunction
Private clsFacetOperator As New ROperator
Private clsFacetRowOp As New ROperator
Private clsFacetColOp As New ROperator
Private clsThemeFunction As New RFunction
Private dctThemeFunctions As New Dictionary(Of String, RFunction)
Private bResetSubdialog As Boolean = True
Expand Down Expand Up @@ -119,6 +117,7 @@ Public Class dlgPICSARainfall
Private clsAesGeomTextLabelUpperTercileLine As New RFunction
Private clsPasteUpperTercileY As New RFunction
Private clsFormatUpperTercileY As New RFunction
Private clsVarsFunction As New RFunction

Private clsAsDate As New RFunction
Private clsAsNumeric As New RFunction
Expand Down Expand Up @@ -199,8 +198,6 @@ Public Class dlgPICSARainfall
ucrInputStation.SetItems({strFacetWrap, strFacetRow, strFacetColAll, strFacetRowAll, strFacetCol, strNone})
ucrInputStation.SetDropDownStyleAsNonEditable()



ucrSave.SetPrefix("picsa_rainfall_graph")
ucrSave.SetIsComboBox()
ucrSave.SetSaveTypeAsGraph()
Expand Down Expand Up @@ -233,16 +230,12 @@ Public Class dlgPICSARainfall
clsPipeOperator = New ROperator
clsFactorLevels = New RFunction

clsVarsFunction = New RFunction
clsCLimitsYContinuous = New RFunction
clsCLimitsYDate = New RFunction

clsFacetFunction = New RFunction
clsFacetOperator = New ROperator
clsFacetRowOp = New ROperator
clsFacetColOp = New ROperator

clsAsDateYLimit = New RFunction

clsGeomHlineMean = New RFunction
clsGeomHlineAesMean = New RFunction
clsMeanFunction = New RFunction
Expand Down Expand Up @@ -320,7 +313,6 @@ Public Class dlgPICSARainfall
'TODO Not yet implemented so do not add
'clsYScaleDateFunction.AddParameter("limits", clsRFunctionParameter:=clsCLimitsYDate, iPosition:=8)


clsThemeFunction = GgplotDefaults.clsDefaultThemeFunction
clsLocalRaesFunction = GgplotDefaults.clsAesFunction.Clone()
dctThemeFunctions = New Dictionary(Of String, RFunction)(GgplotDefaults.dctThemeFunctions)
Expand Down Expand Up @@ -373,14 +365,14 @@ Public Class dlgPICSARainfall
clsPointsFunc.AddParameter("colour", Chr(34) & "red" & Chr(34))

clsFacetFunction.SetPackageName("ggplot2")
clsFacetRowOp.SetOperation("+")
clsFacetRowOp.bBrackets = False
clsFacetColOp.SetOperation("+")
clsFacetColOp.bBrackets = False
'clsFacetRowOp.SetOperation("+")
'clsFacetRowOp.bBrackets = False
'clsFacetColOp.SetOperation("+")
'clsFacetColOp.bBrackets = False
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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

clsFacetOperator.SetOperation("~")
clsFacetOperator.bForceIncludeOperation = True
clsFacetOperator.bBrackets = False
clsFacetFunction.AddParameter("facets", clsROperatorParameter:=clsFacetOperator, iPosition:=0)
'clsFacetFunction.AddParameter("facets", clsROperatorParameter:=clsFacetOperator, iPosition:=0)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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



'Mean Line
Expand Down Expand Up @@ -645,6 +637,9 @@ Public Class dlgPICSARainfall

clsAsNumeric.SetRCommand("as.numeric")

clsVarsFunction.SetPackageName("ggplot2")
clsVarsFunction.SetRCommand("Vars")

clsRaesFunction.AddParameter("y", clsRFunctionParameter:=clsAsNumeric, iPosition:=1)
clsRaesFunction.AddParameter("x", ucrReceiverX.GetVariableNames, iPosition:=2)
clsCoordPolarStartOperator = GgplotDefaults.clsCoordPolarStartOperator.Clone()
Expand Down Expand Up @@ -738,8 +733,8 @@ Public Class dlgPICSARainfall

Private Sub UpdateParameters()
clsFacetOperator.RemoveParameterByName("var1")
clsFacetColOp.RemoveParameterByName("col" & ucrInputStation.Name)
clsFacetRowOp.RemoveParameterByName("row" & ucrInputStation.Name)
'clsFacetColOp.RemoveParameterByName("col" & ucrInputStation.Name)
'clsFacetRowOp.RemoveParameterByName("row" & ucrInputStation.Name)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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

clsBaseOperator.RemoveParameterByName("facets")
bUpdatingParameters = True
ucrReceiverFacetBy.SetRCode(Nothing)
Expand All @@ -749,10 +744,13 @@ Public Class dlgPICSARainfall
ucrReceiverFacetBy.SetRCode(clsFacetOperator)
Case strFacetCol, strFacetColAll
ucrReceiverFacetBy.ChangeParameterName("col" & ucrInputStation.Name)
ucrReceiverFacetBy.SetRCode(clsFacetColOp)
'ucrReceiverFacetBy.SetRCode(clsFacetColOp)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This line is commented out. Please remove it.

clsVarsFunction.AddParameter("var", ucrReceiverFacetBy.GetVariableNames(False), iPosition:=0, bIncludeArgumentName:=False)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This line is commented out. Please remove it.

clsVarsFunction.AddParameter("var", ucrReceiverFacetBy.GetVariableNames(False), iPosition:=0, bIncludeArgumentName:=False)
End Select
If Not clsRaesFunction.ContainsParameter("x") Then
clsRaesFunction.AddParameter("x", Chr(34) & Chr(34))
Expand All @@ -771,6 +769,10 @@ Public Class dlgPICSARainfall
Exit Sub
End If
clsBaseOperator.RemoveParameterByName("facets")
clsFacetFunction.RemoveParameterByName("facets")
clsFacetFunction.RemoveParameterByName("rows")
clsFacetFunction.RemoveParameterByName("ggplot2::rows")
clsFacetFunction.RemoveParameterByName("cols")
If Not ucrReceiverFacetBy.IsEmpty Then
Select Case ucrInputStation.GetText()
Case strFacetWrap
Expand All @@ -786,9 +788,17 @@ Public Class dlgPICSARainfall
bRowAll = True
End Select
End If
If bWrap OrElse bRow OrElse bCol OrElse bColAll OrElse bRowAll Then
clsBaseOperator.AddParameter("facets", clsRFunctionParameter:=clsFacetFunction)

If bWrap Then
clsFacetFunction.AddParameter("facets", clsROperatorParameter:=clsFacetOperator, iPosition:=0)
ElseIf bRow Then
clsFacetFunction.AddParameter("rows", clsRFunctionParameter:=clsVarsFunction)
ElseIf bRowAll Then
clsFacetFunction.AddParameter("ggplot2::rows", clsRFunctionParameter:=clsVarsFunction)
ElseIf bCol OrElse bColAll Then
clsFacetFunction.AddParameter("cols", clsRFunctionParameter:=clsVarsFunction)
End If
clsBaseOperator.AddParameter("facets", clsRFunctionParameter:=clsFacetFunction)

If bWrap Then
clsFacetFunction.SetRCommand("facet_wrap")
Expand All @@ -803,22 +813,21 @@ Public Class dlgPICSARainfall
Else
clsFacetFunction.RemoveParameterByName("margin")
End If

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
'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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

End Sub

Private Sub ucrVariablesAsFactorForPicsa_ControlValueChanged() Handles ucrVariablesAsFactorForPicsa.ControlValueChanged
Expand Down Expand Up @@ -967,10 +976,10 @@ Public Class dlgPICSARainfall
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
Comment on lines 954 to 957
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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

End If
If clsRaesFunction.ContainsParameter("colour") Then
Expand Down Expand Up @@ -1086,3 +1095,4 @@ Public Class dlgPICSARainfall
openSdgLayerOptions(clsPointsFunc)
End Sub
End Class

Loading