Skip to content
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.

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

[solver] add unseccessful perfomance option in DirectInferenceAgent #82

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

piligrinik
Copy link

@piligrinik piligrinik commented Mar 2, 2025

Important

Adds unsuccessful performance handling in DirectInferenceAgent by checking for negative edge types in DoProgram.

  • Behavior:
    • Adds check in DoProgram in DirectInferenceAgent.cpp for unsuccessful target achievement using ScIterator3Ptr.
    • Logs error and returns FinishUnsuccessfully() if EdgeAccessConstNegPerm is found.
  • Misc:
    • Adds comments explaining edge type checks in DoProgram.

This description was created by Ellipsis for 9557d62. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 9557d62 in 1 minute and 32 seconds

More details
  • Looked at 32 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. problem-solver/cxx/inferenceModule/agent/DirectInferenceAgent.cpp:67
  • Draft comment:
    Fix typo in comment: change 'successSoclutionEdge' to 'successSolutionEdge'.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While the typo exists and the comment is technically correct, fixing typos in comments is a very minor issue that doesn't affect functionality. Comments are for documentation purposes only. The rules state not to make comments that are obvious or unimportant, and this seems to fall into that category.
    The typo could potentially confuse future developers reading the code. Documentation quality is important for maintainability.
    While documentation quality matters, this particular typo is minor enough that most readers would easily understand the intended meaning. The cost of the PR review interaction outweighs the benefit.
    Delete this comment as it points out an unimportant typo that doesn't impact code functionality or significantly impair understanding.
2. problem-solver/cxx/inferenceModule/agent/DirectInferenceAgent.cpp:75
  • Draft comment:
    Review iterator logic: if multiple success solution edges exist with mixed types, returning on the first negative edge may be premature. Ensure this is the desired behavior.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the author to ensure that the behavior is intended, which violates the rule against asking for confirmation of intention. It does not provide a specific suggestion or point out a clear issue with the code.
3. problem-solver/cxx/inferenceModule/agent/DirectInferenceAgent.cpp:67
  • Draft comment:
    Typo in comment: change 'successSoclutionEdge' to 'successSolutionEdge'.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While the typo exists and the comment is technically correct, comments about typos in comments are very minor issues. They don't affect functionality and can be fixed during code review without needing a formal comment. This feels like unnecessary noise in the review process.
    The typo could potentially cause confusion for future developers reading the code. Documentation quality is important.
    While documentation is important, this is an extremely minor issue that can be fixed inline during review. The typo is obvious enough that it won't cause real confusion.
    Delete this comment as it's too minor of an issue to warrant a formal PR comment. This falls under the rule about not making obvious or unimportant comments.
4. problem-solver/cxx/inferenceModule/agent/DirectInferenceAgent.cpp:75
  • Draft comment:
    Logic clarification: the code only checks for a negative edge to determine failure. If both negative and positive edges exist, the agent immediately fails. Confirm that this is the intended behavior.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The comment violates the rule about not asking authors to confirm their intention ("Confirm that this is the intended behavior"). Additionally, it's speculative - it points out what could happen in a specific edge case without evidence that this is actually a problem. The code seems to be implementing a straightforward failure-on-negative-edge policy.
    The logic could potentially be buggy if positive edges should take precedence over negative ones. Without understanding the broader system requirements, I can't be 100% sure this isn't a real issue.
    While there might be an edge case here, the comment is phrased as a request for confirmation rather than pointing out a clear bug or suggesting a specific fix. The implementation seems intentional.
    Delete this comment as it violates rules against asking for confirmation and making speculative comments without clear evidence of an actual problem.
5. problem-solver/cxx/inferenceModule/agent/DirectInferenceAgent.cpp:67
  • Draft comment:
    Typo in the comment: "searching for successSoclutionEdge Type" should be corrected to "searching for successSolutionEdge Type".
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While the typo exists and the correction is accurate, comments about typos in comments are generally not important enough to warrant a PR comment. The typo doesn't affect code functionality, and it's a minor issue that could be fixed during code review without needing a formal comment. The rules specifically mention not making comments that are obvious or unimportant.
    The typo could potentially cause confusion for future developers reading the code. Documentation quality is important for maintainability.
    While documentation quality matters, this particular typo is minor enough that readers would still understand the intent, and fixing comment typos doesn't warrant the overhead of a PR comment.
    Delete this comment as it points out a trivial issue that doesn't affect code functionality and falls under the category of "obvious or unimportant" comments that should be avoided.

Workflow ID: wflow_YCqzMRIFLvLtqPY8


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -62,9 +62,26 @@ ScResult DirectInferenceAgent::DoProgram(ScActionInitiatedEvent const & event, S
ScAddr solutionNode = inferenceManager->getSolutionTreeManager()->createSolution(outputStructure, targetAchieved);

action.FormResult(solutionNode);

ScIterator3Ptr successSolutionEdgeIterator =
m_context.CreateIterator3(InferenceKeynodes::concept_success_solution, ScType::Unknown, solutionNode); //searching for successSoclutionEdge Type
Copy link

Choose a reason for hiding this comment

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

Consider specifying the expected edge type in CreateIterator3 instead of using ScType::Unknown for clarity and to avoid ambiguity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant