-
Notifications
You must be signed in to change notification settings - Fork 5
feat: Improve ScoreAnalysis debug information #105
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
feat: Improve ScoreAnalysis debug information #105
Conversation
eb230e9
to
d42ef22
Compare
There was a problem hiding this 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
timefold-solver-python-core/src/main/python/score/_score_analysis.py
Outdated
Show resolved
Hide resolved
timefold-solver-python-core/src/main/python/score/_score_analysis.py
Outdated
Show resolved
Hide resolved
timefold-solver-python-core/src/main/python/score/_score_analysis.py
Outdated
Show resolved
Hide resolved
timefold-solver-python-core/src/main/python/score/_score_analysis.py
Outdated
Show resolved
Hide resolved
timefold-solver-python-core/src/main/python/score/_score_analysis.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
.
timefold-solver-python-core/src/main/python/score/_score_analysis.py
Outdated
Show resolved
Hide resolved
@Christopher-Chianelli @triceo, do we all agree to use the
|
There was a problem hiding this 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 thanmatch.summarize()
, but slightly deviates from the Java name.
I vote for summary
.
@Christopher-Chianelli My rationale for 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? |
I give up. This is not worth the time we already spent on it. @zepfred, please, change it back to |
I was deleting one of my comments and accidentally deleted yours, @Christopher-Chianelli. My apologies, was not intentional. |
No worries @triceo |
There was a problem hiding this 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.
timefold-solver-python-core/src/main/python/score/_score_analysis.py
Outdated
Show resolved
Hide resolved
|
@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! |
This pull request adds a summary logic to the
ScoreAnalysis
andConstraintAnalysis
classes. Additionally, it synchronizes the API of the Python_score_analysis.py
with the Java classes.