fix: replace NaN and Infinity with null in JSON reports#421
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #421 +/- ##
=======================================
Coverage 99.51% 99.51%
=======================================
Files 23 23
Lines 2668 2678 +10
=======================================
+ Hits 2655 2665 +10
Misses 13 13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
58e8789 to
256b959
Compare
sacroml/attacks/report.py
Outdated
| attack_report = attack_report.replace("-Infinity", "null") | ||
| attack_report = attack_report.replace("Infinity", "null") | ||
| attack_report = attack_report.replace("NaN", "null") |
There was a problem hiding this comment.
I believe that:
- this wont catch the case where it's "nan" instead of "NaN"
- if any part of the metrics include this text it will replace them too; try adding
"BaNaNa": "BaNaNa"to the test data for example - it seems safer to handle this problem in the JSON encoder?
The code does not match your comments. |
2efd053 to
09b9a6e
Compare
Apologies for any errors I have made on this issue, I have tried to implement your suggested changes, please let me know your thoughts @rpreen |
|
I don't think this will work for numpy arrays. Try adding the test case: "array": np.array([1.0, np.nan, np.inf])which should assert to the following: assert inner["array"] == [1.0, None, None] |
Changes made and updated branch. I couldn't think of a different way other than the one you suggested for numpy array handling but lgtm. Let me know your thoughts @rpreen |
There was a problem hiding this comment.
This looks reasonable to me.
@rpreen ?
For reference, I encountered this problem when looking at results.json produced from running attacks on some very vulnerable xgboost models.
it was always the first entries in the roc_thresholds entries - which probably arise from a divide by zero issue
rpreen
left a comment
There was a problem hiding this comment.
lgtm; thanks for making the changes
|
You need to add your details to the |
Closes #366
JSON reports produced by attacks could contain bare
NaN,Infinity,and
-Infinitytokens which are invalid per the JSON spec and causeparse errors in browsers.
Changes:
CustomJSONEncoder.default()write_json()via.replace()null