Skip to content

feat: Improve ScoreAnalysis debug information #923

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

Jump to bottom
Merged
merged 9 commits into from
Jul 5, 2024
Merged

feat: Improve ScoreAnalysis debug information #923

merged 9 commits into from
Jul 5, 2024

Conversation

zepfred
Copy link
Contributor

@zepfred zepfred commented Jun 27, 2024 •
edited
Loading

This pull request adds a summary logic to the ScoreAnalysis class and addresses minor issues.

@zepfred zepfred linked an issue Jun 27, 2024 that may be closed by this pull request
ScoreAnalysis.summary() for debugging purposes #883
Closed
3 tasks
@zepfred zepfred requested a review from triceo June 27, 2024 12:06
@zepfred zepfred marked this pull request as draft June 27, 2024 12:12
@zepfred zepfred changed the title feat: Add score analysis summary feat: Improve ScoreAnalysis debug information Jun 27, 2024
This was linked to issues Jun 27, 2024
ConstraintAnalysis: add matchCount() or matchesSize() #885
Closed
ScoreAnalysis should not fail fast if the solver is unintialized #903
Closed
@zepfred zepfred marked this pull request as ready for review June 28, 2024 12:02
@zepfred zepfred force-pushed the score-analysis-summary branch from 98c5fcf to 1e39882 Compare June 28, 2024 12:06
triceo
triceo requested changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@triceo triceo left a comment

Choose a reason for hiding this comment

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

Good start!
Please double-check serialization of the analysis in persistence-jackson. Nothing should have changed.

@triceo triceo modified the milestone: v1.12.0 Jul 1, 2024
@triceo
Copy link
Contributor

triceo commented Jul 1, 2024

Also, it makes sense to fix this.

@zepfred zepfred force-pushed the score-analysis-summary branch from 83fd75b to 1a3fd22 Compare July 1, 2024 12:50
@zepfred zepfred requested a review from triceo July 1, 2024 12:50
triceo
triceo approved these changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@triceo triceo left a comment

Choose a reason for hiding this comment

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

There are some new Sonar warnings which I think are good to address.
Other than that, LGTM.

@zepfred
Copy link
Contributor Author

zepfred commented Jul 1, 2024

There are some new Sonar warnings which I think are good to address. Other than that, LGTM.

I remember encountering errors when using %n on Mac. Let me attempt it again.

@zepfred
Copy link
Contributor Author

zepfred commented Jul 1, 2024

There are some new Sonar warnings which I think are good to address. Other than that, LGTM.

I remember encountering errors when using %n on Mac. Let me attempt it again.

Here is the problem:

https://github.com/TimefoldAI/timefold-solver/actions/runs/9750701051/job/26910684905?pr=923#step:4:2824

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Jul 1, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
88.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@Christopher-Chianelli Christopher-Chianelli mentioned this pull request Jul 5, 2024
Merged
@Christopher-Chianelli Christopher-Chianelli merged commit 7e6102e into TimefoldAI:main Jul 5, 2024
24 checks passed
@zepfred zepfred deleted the score-analysis-summary branch July 5, 2024 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@triceo triceo triceo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@zepfred @triceo @Christopher-Chianelli