chore: introduce structural component to score - #2583
chore: introduce structural component to score#2583Christopher-Chianelli wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Leaving comments. Comments on any score type should be considered to apply to every other score type as well - I didn't want to duplicate every comment 10 times.
Score documentation needs updating; we need to explain what is a structural score, what it's for, and how it's handled. (If you prefer, can wait for a later PR.)
Consider adding migration recipes as much as you can. Going forward, we will try to make every change to happen automatically. However, automated or not, please update the recipes in the docs to mention all the changes.
Also, I would like to see tests added; structural score is an important part of the score types, we should test for it. The existing coverage should be easily extendable.
| public record BendableBigDecimalScore(long structuralScore, BigDecimal[] hardScores, | ||
| BigDecimal[] softScores) implements IBendableScore<BendableBigDecimalScore> { | ||
|
|
||
| public BendableBigDecimalScore(BigDecimal[] hardScores, |
There was a problem hiding this comment.
Let's deprecate these convenience constructors to make it clear that the canonical constructor is still the way to go.
There was a problem hiding this comment.
Not sure about that; the solver sets the structural score, not score calculation, so for the vast majority, if not all, cases, users will do Score(0L, ...).
There was a problem hiding this comment.
Does the user need to create a score? Should they? IMO no.
I'd even go as far as to write in the Javadoc that the constructor is not considered public API, as the user has no need to create Score instances.
| SimpleBigDecimalScore, SimpleScore { | ||
| /** | ||
| * The structural component of a Score. 0 if the solution is structurally sound | ||
| * (i.e. no inconsistent variables), -1 otherwise. |
There was a problem hiding this comment.
I would specifically mention that this is effectively a superhard score - it is the most important component of any score.
triceo
left a comment
triceo
left a comment
There was a problem hiding this comment.
LGTM, with comments.
If you want to leave docs for later, that's your call, but in that case I recommend keeping a list of things to document; it's growing long.
| public String toString() { | ||
| return hardScore + HARD_LABEL + "/" + mediumScore + MEDIUM_LABEL + "/" + softScore + SOFT_LABEL; | ||
| return (structuralScore < 0) | ||
| ? "%dstructural/%s%s/%s%s/%s%s".formatted(structuralScore, hardScore, HARD_LABEL, mediumScore, MEDIUM_LABEL, |
There was a problem hiding this comment.
We have String constants for hard/medium/soft, let's introduce a constant for "structural" as well.
| @Override | ||
| public String toString() { | ||
| return Long.toString(score); | ||
| return (structuralScore < 0) ? "%dstructural/%d".formatted(structuralScore, score) : Long.toString(score); |
| var scoreTokens = new String[2][]; | ||
| var scoreTokens = new String[3][]; | ||
| var startIndex = 0; | ||
| var structuralSlashIndex = scoreString.indexOf("structural/"); |
| var structuralSlashIndex = scoreString.indexOf("structural/"); | ||
| if (structuralSlashIndex >= 0) { | ||
| scoreTokens[0] = new String[] { scoreString.substring(0, structuralSlashIndex) }; | ||
| startIndex = structuralSlashIndex + "structural/".length(); |
2 participants
No description provided.