Skip to content
This repository has been archived by the owner on Jul 17, 2024. It is now read-only.

feat: Improve ScoreAnalysis debug information #105

Merged
merged 16 commits into from
Jul 5, 2024

Conversation

zepfred
Copy link
Contributor

@zepfred zepfred commented Jul 2, 2024

This pull request adds a summary logic to the ScoreAnalysis and ConstraintAnalysis classes. Additionally, it synchronizes the API of the Python _score_analysis.py with the Java classes.

@zepfred
Copy link
Contributor Author

zepfred commented Jul 2, 2024

@triceo I created a new pull request because the branch was renamed to run the tests correctly. Your comments from #102 have been addressed.

@zepfred zepfred marked this pull request as draft July 2, 2024 21:18
@zepfred zepfred requested review from triceo and Christopher-Chianelli and removed request for triceo and Christopher-Chianelli July 2, 2024 21:18
@triceo triceo removed their request for review July 3, 2024 05:23
@zepfred zepfred marked this pull request as ready for review July 3, 2024 14:57
Copy link
Collaborator

@Christopher-Chianelli Christopher-Chianelli left a comment

Choose a reason for hiding this comment

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

  • Do not use method names as attribute names
  • Do not add additional dependencies

Copy link
Collaborator

@Christopher-Chianelli Christopher-Chianelli left a comment

Choose a reason for hiding this comment

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

I opt for removing indicted_object_list, since it just a rename of indicted_objects.

@zepfred
Copy link
Contributor Author

zepfred commented Jul 5, 2024

@Christopher-Chianelli @triceo, do we all agree to use the summarize as a method?

def assert_score_analysis_summary(score_analysis: ScoreAnalysis):
    summary = score_analysis.summarize()
    ...
    match = score_analysis.constraint_analyses[0]
    match_summary = match.summarize()
    ...

Copy link
Collaborator

@Christopher-Chianelli Christopher-Chianelli left a comment

Choose a reason for hiding this comment

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

@triceo summary property or summarize method?

  • match.summary is more pythonic than match.summarize(), but slightly deviates from the Java name.

I vote for summary.

@triceo
Copy link
Contributor

triceo commented Jul 5, 2024

@Christopher-Chianelli My rationale for summarize is that this isn't actually an accessor for a property, it is not a constant - in that case, arguably summary would be correct. But summarize is a method that computes a result when it is called, which I think warrants the different name. IMO it has nothing to do with Python or Java.

Are you telling me that in Python, they don't actually make a distinction between methods and properties, even though they clearly have those constructs?

@triceo
Copy link
Contributor

triceo commented Jul 5, 2024

I give up. This is not worth the time we already spent on it.

@zepfred, please, change it back to summary and make it a property. I'm sorry for this.

@triceo
Copy link
Contributor

triceo commented Jul 5, 2024

I was deleting one of my comments and accidentally deleted yours, @Christopher-Chianelli. My apologies, was not intentional.

@Christopher-Chianelli
Copy link
Collaborator

No worries @triceo

Copy link
Collaborator

@Christopher-Chianelli Christopher-Chianelli left a comment

Choose a reason for hiding this comment

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

API looks good, I would change the IndexError to a ValueError; IndexError are typically used for invalid indexes, whereas ValueError are used for invalid values.

Copy link

sonarcloud bot commented Jul 5, 2024

@Christopher-Chianelli Christopher-Chianelli merged commit 4396e2d into TimefoldAI:main Jul 5, 2024
8 checks passed
@zepfred zepfred deleted the score-analysis-summary branch July 5, 2024 19:38
@Christopher-Chianelli
Copy link
Collaborator

@zepfred Please state clearly when a PR depends on another PR; in particular, this PR shouldn't have been merged before TimefoldAI/timefold-solver#923

@zepfred
Copy link
Contributor Author

zepfred commented Jul 5, 2024

@zepfred Please state clearly when a PR depends on another PR; in particular, this PR shouldn't have been merged before TimefoldAI/timefold-solver#923

Understood!

@zepfred zepfred linked an issue Jul 5, 2024 that may be closed by this pull request
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

print(ScoreAnalysis): make it easy to analyze a score during POC
3 participants