Improve use of already calculated constraints #116
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lation, working on percentage setting of dose quantity
…ated constraints, remove need to recacl min-max
Greptile OverviewGreptile SummaryThis PR introduces a calculated constraints tracking system to improve order processing efficiency. The key architectural change is adding a Key Changes:
Issues Found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant OrderProcessor
participant Order
participant OrderVariable
participant Variable
Note over Client,Variable: Calculated Constraints Flow
Client->>OrderProcessor: CalcMinMax Command
OrderProcessor->>Order: applyConstraints()
Order->>OrderVariable: applyConstraints()
OrderVariable->>OrderVariable: Check if Calculated is set
alt Calculated constraints exist
OrderVariable->>OrderVariable: applyCalculatedConstraints()
else No calculated constraints
OrderVariable->>OrderVariable: Apply regular Constraints
end
OrderProcessor->>Order: calcMinMax()
Order->>Variable: solveMinMax()
Variable-->>Order: Solved order with min/max
OrderProcessor->>Order: setCalculatedConstraints()
Order->>OrderVariable: setCalculatedConstraints()
OrderVariable->>OrderVariable: Store Variable.Values as Calculated
OrderProcessor->>Order: increaseIncrements()
Order->>Variable: increaseIncrement()
Variable->>Variable: Filter increments > current
Variable-->>Order: Updated increments
OrderProcessor->>Order: solveNormDose()
Order->>Order: hasNormDose()
alt Has norm dose
Order->>Order: setNormDose()
end
Order-->>Client: Result with calculated constraints
Note over Client,Variable: Property Change Flow
Client->>OrderProcessor: ChangeProperty Command
OrderProcessor->>OrderProcessor: processChangeProperty()
OrderProcessor->>Order: Apply property changes
Order->>OrderVariable: setPercValue() / setMinValue() / etc
OrderProcessor->>Order: calcMinMax()
Order-->>Client: Updated order
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+69
to
+73
| // |> solveMinMax "Order Property Set Frequency" printErr logger | ||
| // step to a min, median or max value | ||
| |> Result.map (OrderPropertyChange.proc [ ScheduleFrequency step ]) | ||
| // |> Result.map (OrderPropertyChange.proc [ ScheduleFrequency step ]) | ||
| |> OrderPropertyChange.proc [ ScheduleFrequency step ] | ||
| |> Ok |
There was a problem hiding this comment.
commented-out code should be removed rather than kept in the codebase
Suggested change
| // |> solveMinMax "Order Property Set Frequency" printErr logger | |
| // step to a min, median or max value | |
| |> Result.map (OrderPropertyChange.proc [ ScheduleFrequency step ]) | |
| // |> Result.map (OrderPropertyChange.proc [ ScheduleFrequency step ]) | |
| |> OrderPropertyChange.proc [ ScheduleFrequency step ] | |
| |> Ok | |
| |> OrderPropertyChange.proc [ ScheduleFrequency step ] | |
| |> Ok |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Informedica.GenORDER.Lib/OrderProcessor.fs
Line: 69:73
Comment:
commented-out code should be removed rather than kept in the codebase
```suggestion
|> OrderPropertyChange.proc [ ScheduleFrequency step ]
|> Ok
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
112
to
+117
| // re-calc the min max | ||
| |> solveMinMax "Order Property Set Dose Rate" printErr logger | ||
| // |> solveMinMax "Order Property Set Dose Rate" printErr logger | ||
| // step to a min, median or max rate | ||
| |> Result.map (OrderPropertyChange.proc [ OrderableDose step ]) | ||
|
|
||
| // |> Result.map (OrderPropertyChange.proc [ OrderableDose step ]) | ||
| |> OrderPropertyChange.proc [ OrderableDose step ] | ||
| |> Ok |
There was a problem hiding this comment.
commented-out code should be removed
Suggested change
| // re-calc the min max | |
| |> solveMinMax "Order Property Set Dose Rate" printErr logger | |
| // |> solveMinMax "Order Property Set Dose Rate" printErr logger | |
| // step to a min, median or max rate | |
| |> Result.map (OrderPropertyChange.proc [ OrderableDose step ]) | |
| // |> Result.map (OrderPropertyChange.proc [ OrderableDose step ]) | |
| |> OrderPropertyChange.proc [ OrderableDose step ] | |
| |> Ok | |
| |> OrderPropertyChange.proc [ OrderableDose step ] | |
| |> Ok |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Informedica.GenORDER.Lib/OrderProcessor.fs
Line: 112:117
Comment:
commented-out code should be removed
```suggestion
|> OrderPropertyChange.proc [ OrderableDose step ]
|> Ok
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
169
to
+176
| OrderableDoseCount OrderVariable.Count.applyConstraints | ||
| ] | ||
| // re-calc min max | ||
| |> solveMinMax "Order Property Set Dose Quantity" printErr logger | ||
| // |> solveMinMax "Order Property Set Dose Quantity" printErr logger | ||
| // step to a min, median or max dose quantity | ||
| |> Result.map (OrderPropertyChange.proc [ OrderableDose step ]) | ||
| // |> Result.map (OrderPropertyChange.proc [ OrderableDose step ]) | ||
| |> OrderPropertyChange.proc [ OrderableDose step ] | ||
| |> Ok |
There was a problem hiding this comment.
commented-out code should be removed
Suggested change
| OrderableDoseCount OrderVariable.Count.applyConstraints | |
| ] | |
| // re-calc min max | |
| |> solveMinMax "Order Property Set Dose Quantity" printErr logger | |
| // |> solveMinMax "Order Property Set Dose Quantity" printErr logger | |
| // step to a min, median or max dose quantity | |
| |> Result.map (OrderPropertyChange.proc [ OrderableDose step ]) | |
| // |> Result.map (OrderPropertyChange.proc [ OrderableDose step ]) | |
| |> OrderPropertyChange.proc [ OrderableDose step ] | |
| |> Ok | |
| |> OrderPropertyChange.proc [ OrderableDose step ] | |
| |> Ok |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Informedica.GenORDER.Lib/OrderProcessor.fs
Line: 169:176
Comment:
commented-out code should be removed
```suggestion
|> OrderPropertyChange.proc [ OrderableDose step ]
|> Ok
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+198
to
+204
| ] | ||
| // re-calc min max | ||
| // |> solveMinMax "Order Property Set Dose Quantity" printErr logger | ||
| // step to a min, median or max dose quantity | ||
| // |> Result.map (OrderPropertyChange.proc [ OrderableDose step ]) | ||
| |> OrderPropertyChange.proc [ OrderableDose step ] | ||
| |> Ok |
There was a problem hiding this comment.
commented-out code should be removed
Suggested change
| ] | |
| // re-calc min max | |
| // |> solveMinMax "Order Property Set Dose Quantity" printErr logger | |
| // step to a min, median or max dose quantity | |
| // |> Result.map (OrderPropertyChange.proc [ OrderableDose step ]) | |
| |> OrderPropertyChange.proc [ OrderableDose step ] | |
| |> Ok | |
| |> OrderPropertyChange.proc [ OrderableDose step ] | |
| |> Ok |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Informedica.GenORDER.Lib/OrderProcessor.fs
Line: 198:204
Comment:
commented-out code should be removed
```suggestion
|> OrderPropertyChange.proc [ OrderableDose step ]
|> Ok
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+724
to
+725
| os.OrderIsSolved |> not //&& | ||
| //os.HasValues |> not |
There was a problem hiding this comment.
commented-out condition should be removed or documented
Suggested change
| os.OrderIsSolved |> not //&& | |
| //os.HasValues |> not | |
| os.OrderIsSolved |> not |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Informedica.GenORDER.Lib/OrderProcessor.fs
Line: 724:725
Comment:
commented-out condition should be removed or documented
```suggestion
os.OrderIsSolved |> not
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
329
to
+331
| |> Result.map (OrderPropertyChange.proc [ ComponentOrderableQuantity (cmp, step) ]) | ||
| // |> OrderPropertyChange.proc [ ComponentOrderableQuantity (cmp, step) ] | ||
| // |> Ok |
There was a problem hiding this comment.
commented-out code should be removed
Suggested change
| |> Result.map (OrderPropertyChange.proc [ ComponentOrderableQuantity (cmp, step) ]) | |
| // |> OrderPropertyChange.proc [ ComponentOrderableQuantity (cmp, step) ] | |
| // |> Ok | |
| |> Result.map (OrderPropertyChange.proc [ ComponentOrderableQuantity (cmp, step) ]) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Informedica.GenORDER.Lib/OrderProcessor.fs
Line: 329:331
Comment:
commented-out code should be removed
```suggestion
|> Result.map (OrderPropertyChange.proc [ ComponentOrderableQuantity (cmp, step) ])
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: src/Informedica.GenORDER.Lib/OrderProcessor.fs
Line: 282:331
Comment:
inconsistent error handling approach - other property setter functions (`orderPropertySetFrequency`, `orderPropertySetDoseRate`, `orderPropertySetDoseQuantity`) now return `Ok` directly without calling `solveMinMax`, but this function still uses `Result.bind` pattern. Consider aligning with the new pattern or documenting why this needs different handling
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request introduces several enhancements and refactorings to the
OrderandOrderProcessormodules, focusing on improved constraint handling, dose normalization, and table output formatting. It adds new utility functions for applying and setting calculated constraints, refines how orders are processed when properties change, and streamlines table output for better logging and debugging. Additionally, new script files are included in the project.Constraint and Dose Handling Improvements:
applyOnlyMaxConstraints,applyCalculatedConstraints, andsetCalculatedConstraintsfunctions to theOrdermodule, enabling more precise application and management of constraints on doses and orders. [1] [2] [3]hasNormDoseand improvedsetNormDoselogic to better handle normalization of doses based on order schedules and components. [1] [2]Order Property Processing Enhancements:
OrderProcessorto use new constraint application functions and to process property changes in a more functional, composable way (e.g., always returningOk, not using intermediateResult.map). AddedorderPropertySetDoseQuantityPercfor setting dose quantity by percentage. [1] [2] [3]Console Table and Logging Refactor:
printTablewithtoConsoleTable, addedtoConsoleTableString, and updated logging to use these new functions for clearer, more informative output. [1] [2] [3] [4] [5]Increment and Calculation Adjustments:
@greptile: review
calcMinMaxto remove theincreaseIncrementparameter. [1] [2] [3] [4]Project Structure:
Constraints.fsxandNormDose.fsxto the project file for improved testing and documentation.