Skip to content

Conversation

@hssqz
Copy link

@hssqz hssqz commented Nov 24, 2025

Summary

Fixes the financial dashboard creation example in
02_skills_financial_applications.ipynb (cell-12) to pass complete
financial statement data to the LLM instead of only revenue data.

Problem

Currently, only revenue data is extracted and passed to the LLM prompt:

  • The fs_data variable is defined but never used
  • Only revenue_by_quarter is extracted from the financial statements
  • The prompt references Gross Profit, Operating Income, and Net Income
    but doesn't provide actual values
  • This causes the LLM to generate estimated/fictional data instead of
    using real values from the dataset

Changes

  • Updated the prompt to include complete financial statement data from
    all categories (Revenue, Gross Profit, Operating Income, Net Income)
  • Ensured the LLM uses actual quarterly values (Q1-Q4 2024) for all
    financial metrics
  • Removed unused variable and simplified data passing

Impact

  • Users following this example will now see how to properly pass
    complete financial data to the LLM
  • Generated Excel files will contain actual financial values instead of
    estimates
  • Better demonstrates real-world financial reporting use cases

Test Plan

  • Run the updated cell-12 in the notebook
  • Verify that generated Excel contains actual values for all
    financial categories
  • Confirm no unused variables remain

Related issue: #298

Copy link
Collaborator

@PedramNavid PedramNavid left a comment

Choose a reason for hiding this comment

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

Code Review: PR #300

Overall Assessment: Feedback Provided

Summary: This PR aims to fix the financial dashboard creation example to pass complete financial statement data to the LLM instead of only revenue data. The core fix is correct, but there are several items to consider before merging.


Issues to Address

1. Incomplete refactoring (High Priority)

Location: Cell 12, lines 288-294

The revenue_by_quarter extraction is still present and used in the prompt, even though complete data is now being passed via financial_data_2024.to_string(). This creates redundancy.

Recommendation: Either remove revenue_by_quarter entirely and update the prompt to reference the complete data, or add a comment explaining why both are needed.

2. Commented-out code should be removed

Location: Cell 12, line 289

# fs_data = financial_statements.to_dict("records")

Commented-out code should not be committed. Please remove this line.

3. Extra blank line

Location: Cell 12, line 292

Two consecutive blank lines after financial_data_2024 assignment. Consider removing one to follow PEP 8 style.

4. Unrelated markdown formatting changes

Location: Cells 0 and 23

The PR converts single-line markdown strings to multi-line arrays, which is purely cosmetic and adds ~60 lines of noise to the diff. Consider reverting these changes or doing them in a separate formatting-only PR to keep this PR focused.


Suggestions

  • Add explanatory comment about why complete data is being passed
  • Consider .to_markdown(index=False) instead of .to_string() for potentially better LLM comprehension

Positive Notes

  • ✅ Core issue correctly identified and fix approach is sound
  • ✅ Proper pandas column selection
  • ✅ Clear prompt addition: "Here is the complete 2024 financial data for reference:"
  • ✅ Added newline at EOF

🤖 Generated with Claude Code

Copy link
Collaborator

@PedramNavid PedramNavid left a comment

Choose a reason for hiding this comment

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

hi @hssqz thanks for the PR! Please review the comments above. There's a couple important ones. Also make sure to run ruff format and ruff check as a final check before submitting! Thanks again

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.

2 participants