Skip to content

Report fixes#63

Merged
machshev merged 4 commits intolowRISC:masterfrom
machshev:report-fixes
Nov 21, 2025
Merged

Report fixes#63
machshev merged 4 commits intolowRISC:masterfrom
machshev:report-fixes

Conversation

@machshev
Copy link
Copy Markdown
Collaborator

@machshev machshev commented Nov 19, 2025 •
edited
Loading

Some bug introduced with the new report template.

  • Fix JSON link on summary page
  • Variant missing

Fixes: #65

@machshev
fix: summary json link name
34fd4ff 
Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
@machshev
refactor: improve data models
a088167 
Make sure variant is available for report generation. This is done by
reusing existing models better. We can still tidy this up further, but
this gets us to a bit better place.

Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
@machshev
fix: restore variant to the report
557a89a 
Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
@machshev
fix: upper case the block names to match the previous reports
25837ce 
Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
@machshev machshev marked this pull request as ready for review November 20, 2025 13:03
etterli
etterli reviewed Nov 20, 2025
View reviewed changes
Copy link
Copy Markdown

@etterli etterli left a comment

Choose a reason for hiding this comment

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

Seems good to me (I'm new to dvsim)

@etterli etterli self-requested a review November 20, 2025 17:18
martin-velay
martin-velay approved these changes Nov 21, 2025
View reviewed changes
Copy link
Copy Markdown

@martin-velay martin-velay left a comment

Choose a reason for hiding this comment

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

I am not a Python specialist but from what I understand it looks good. As long as it works well I am satisfied with these changes.

@machshev machshev added this pull request to the merge queue Nov 21, 2025
Merged via the queue into lowRISC:master with commit 8f2e2eb Nov 21, 2025
12 checks passed
@machshev machshev deleted the report-fixes branch November 21, 2025 10:48
@machshev
Copy link
Copy Markdown
Collaborator Author

machshev commented Nov 21, 2025 •
edited
Loading

Thanks @etterli and @martin-velay!
It's a step in the right direction hopefully. I'm slowly moving the data into pydantic data classes and hopefully grouping them more logically. There is currently quite a bit of duplication in the data classes, so it would be nice to consolidate a bit more in the future.

Additionally some of the data should be calculated earlier in the flow and then referenced. But due to the way the wildcards are calculated that isn't easy at the moment. Hopefully when the earlier stages of the flow can be refactored this will be possible.

The report generation bug appears to be resolved with these changes when I test it locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martin-velay martin-velay martin-velay approved these changes

@hcallahan-lowrisc hcallahan-lowrisc Awaiting requested review from hcallahan-lowrisc

@rswarbrick rswarbrick Awaiting requested review from rswarbrick

@engdoreis engdoreis Awaiting requested review from engdoreis

@nbdd0121 nbdd0121 Awaiting requested review from nbdd0121

@AlexJones0 AlexJones0 Awaiting requested review from AlexJones0

@nasahlpa nasahlpa Awaiting requested review from nasahlpa

+1 more reviewer

@etterli etterli etterli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Variant incorrectly handled with the new report system

3 participants

@machshev @etterli @martin-velay