[MRG] IS float #1720
[MRG] IS float #1720
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1720 +/- ##
==========================================
+ Coverage 97.58% 97.60% +0.01%
==========================================
Files 66 66
Lines 10744 10769 +25
==========================================
+ Hits 10485 10511 +26
+ Misses 259 258 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| config.strict_reading() if suppress_invalid_tags | ||
| else nullcontext() | ||
| ) | ||
| with context: |
There was a problem hiding this comment.
Allowing ISfloat by default caused some json tests to fail. Added this strict context to try to keep previous behavior.
There was a problem hiding this comment.
I have some trouble understanding the logic here: if suppress_invalid_tags is set, we raise an exception, and will be cught and ignored here? I'll probably have another look tomorrow with a fresh brain...
There was a problem hiding this comment.
I checked the failing test (test_suppress_invalid_tags_with_failed_dataelement) - I wrote that, and I think it just should be changed to use another invalid value that would still raise, for example:
ds[0x00082128] = RawDataElement(
Tag(0x00082128), 'IS', 4, b'5:25', 0, True, True
)
with pytest.warns(UserWarning):
ds_json = ds.to_json_dict(suppress_invalid_tags=True)
assert "00082128" not in ds_jsonand revert the change here.
There was a problem hiding this comment.
...I think it just should be changed to use another invalid value that would still raise
I thought about that too, but that would still change existing behavior for IS - ones that did not get written before would now be accepted and written out. Probably not a big deal, but still something a bit different. I'm not sure just skipping values in the output is the best idea anyway. I'll think about this a bit before this is merged. I'd still like to have a "bigger picture" in mind for how to handle settings in v3.X... I'll start an issue and post some thoughts on that in the coming days.
There was a problem hiding this comment.
but that would still change existing behavior for IS - ones that did not get written before would now be accepted and written out
That is true, but I assumed that this is a change we want to make. Though maybe I haven't thought this through... I will wait your thoughts then.
There was a problem hiding this comment.
Having a look at this again ... This change has no effect (nullcontext()) for the default case where suppress_invalid_tags is False. So I think I was right, if suppress... is True (as seen only in tests probably at the moment), then we will catch the exception and ignore it, as is currently done (the new ISFloat is not allowed with strict_reading() turned on).
It is still messy, but I'd rather push real fixes to this messy config (and all the other messy config) to v3.0.
There was a problem hiding this comment.
Well, I leave the decision to you. I find it a bit ugly, but you are probably right - better defer the real fix to 3.0, where behavior changes are expected.
| bin_elem = b"\x18\x00\x52\x11\x04\x00\x00\x0014.5" | ||
| with BytesIO(bin_elem) as bio: | ||
| ds = read_dataset(bio, True, True) | ||
| assert isinstance(ds.Exposure, ISfloat) |
There was a problem hiding this comment.
Not strictly necessary to go back to binary like this rather than just creating an IS, but I thought it a good idea in case of future hooks into reading invalid values.
| _ = IS("14.5", validation_mode=config.RAISE) | ||
| with pytest.raises(TypeError): | ||
| _ = IS(14.5, validation_mode=config.RAISE) | ||
|
|
There was a problem hiding this comment.
This seems a bit odd to me to have two different Errors for the same value, but that is the way it is done currently. Perhaps to be revisited for the grand redesign of validation settings?
There was a problem hiding this comment.
I remember that I was not happy about this either, though forgot why I didn't change it...
There was a problem hiding this comment.
In part I guess there are two branches, so to speak, because some are by string processing (regex) and others are by value. Another thing to think about in harmonizing validation.
| config.strict_reading() if suppress_invalid_tags | ||
| else nullcontext() | ||
| ) | ||
| with context: |
There was a problem hiding this comment.
I have some trouble understanding the logic here: if suppress_invalid_tags is set, we raise an exception, and will be cught and ignored here? I'll probably have another look tomorrow with a fresh brain...
| bin_elem = b"\x18\x00\x52\x11\x04\x00\x00\x0014.5" | ||
| with BytesIO(bin_elem) as bio: | ||
| ds = read_dataset(bio, True, True) | ||
| assert isinstance(ds.Exposure, ISfloat) |
| _ = IS("14.5", validation_mode=config.RAISE) | ||
| with pytest.raises(TypeError): | ||
| _ = IS(14.5, validation_mode=config.RAISE) | ||
|
|
There was a problem hiding this comment.
I remember that I was not happy about this either, though forgot why I didn't change it...
| # If a string passed, then store it | ||
| if isinstance(val, str): | ||
| self.original_string = val.strip() | ||
| elif isinstance(val, (IS, ISfloat)) and hasattr(val, 'original_string'): |
There was a problem hiding this comment.
I recently learned that black uses 88 instead of 79 characters as a default maximum line length, and I tend to agree with it... I think we had this discussion before, and it is not really important, though.
There was a problem hiding this comment.
I was going to raise this point as well. I find 79 very limiting, it would really help to have even 9 more characters, and useful to just let black do its thing on new code before posting a PR.
We can change to 88 in a separate PR maybe, and update the documentation for contributors.
There was a problem hiding this comment.
Yes, of course - this was not meant for this PR, just wanted to mention it.
Describe the changes
Closes #1661. Introduces new ISfloat class which is used if a non-integer float is passed to IS and settings allow it.
Tasks
doc/_build/html/index.html)