Skip to content

Fix incorrect JSON source for item_defect_minimum_spacing#318

Merged
fontanf merged 2 commits intofontanf:masterfrom
findux:master
Mar 16, 2026
Merged

Fix incorrect JSON source for item_defect_minimum_spacing#318
fontanf merged 2 commits intofontanf:masterfrom
findux:master

Conversation

@findux
Copy link
Contributor

@findux findux commented Mar 9, 2026

Read item_defect_minimum_spacing from json_defect instead of json_item.

Read item_defect_minimum_spacing from json_defect instead of json_item.
@fontanf
Copy link
Owner

fontanf commented Mar 9, 2026

Hi,

Thank you for the contribution.

It looks good.

Do you think you could include a unit test by adding corresponding instance/expected solution files like here https://github.com/fontanf/packingsolver/tree/master/data/irregular/tests and adding them as test at the end of this file https://github.com/fontanf/packingsolver/blob/master/test/irregular/irregular_test.cpp ? Otherwise, I'll try to do it myself later

@findux
Copy link
Contributor Author

findux commented Mar 9, 2026

Hi,

Thank you for the review.

I can provide the instance JSON file that exposed the issue (the one you shared earlier). instance.json
If needed, I can also help integrate it into the test suite.

Since the change only corrects the JSON source for a single parameter, I wasn't entirely sure how such tests are usually structured in this project. If you prefer to add the test on your side, that would also be perfectly fine for me.

Best regards

@fontanf
Copy link
Owner

fontanf commented Mar 9, 2026

Please integrate the test if you can. As I mentioned above, you just need to solve the problem to get the solution file, store both the instance and the solution files in data/irregular/tests, add the corresponding test at the end of test/irregular/irregular_test.cpp, and run the tests with ctest --output-on-failure --parallel --test-dir build/test/ to check that everything is good. Otherwise, I'll do it myself later

@fontanf fontanf merged commit 4b5d527 into fontanf:master Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants