Repository navigation
Fix EasyList serialization issue and add test - #323
Conversation
damskii9992
added
[scope] bug
rozyczko
left a comment
rozyczko
left a comment
There was a problem hiding this comment.
Lots of docstring changes muddling the actual code fix.
Anyway, there's a funcitonality deterioration for an edge case, so consider fixing it please.
| ) # noqa: E501 | ||
| else: | ||
| protected_types = None | ||
| data_dicts = temp_dict.pop('data', None) |
There was a problem hiding this comment.
Missing data key will now crash this.
EasyList pops with a default of None and then iterates over it. In the previous code a dict without data deserialized to an empty list. Here, it will raise TypeError. Maybe Use [] as the default?
data_dicts = temp_dict.pop('data', [])
This is how to show it:
from easyscience.base_classes.easy_list import EasyList
d = EasyList(unique_name='x').to_dict()
d.pop('data')
EasyList.from_dict(d)develop just returns an empty string
There was a problem hiding this comment.
Well, is that really an issue? If the dict doesn't have a data key, then it isn't a serialized EasyList . . .
An empty EasyList will have an empty list in its data key.
I CAN change it, but do we want faulty dicts to properly deserialize?
There was a problem hiding this comment.
I should probably remove the default though. Better to fail with "missing key" error than "can't iterate over None" error.
There was a problem hiding this comment.
Nothing inthe code builds an EasyList dict by hand, and to_dict always writes the key, so a missing data means the input is malformed. I guess failing is the right thing. But I would argue that KeyError is inconsistent. Every other bad input raises ValueError saying why the input is wrong. Let's be consistent.
something like
if 'data' not in temp_dict:
raise ValueError("Serialized EasyList is missing the 'data' key.")
data_dicts = temp_dict.pop('data')| fitter : Fitter | ||
| A configured ``Fitter`` (or ``MultiFitter``) whose minimizer has been | ||
| switched to ``AvailableMinimizers.Bumps``. | ||
| fitter : 'Fitter' |
There was a problem hiding this comment.
Are the quotes around Fitter correct? Will they render properly?
There was a problem hiding this comment.
Nope. But it was also wrong before lol. I don't know what prettier did, but it changed stuff.
We need to do a proper overhaul of the EasyScience documentation sometime after the 3.0 release . . .
There was a problem hiding this comment.
Actually, never mind, it did change it for the worse . . .
| an increment: to extend an existing chain of ``N`` raw | ||
| samples by ``M``, pass ``samples=N + M`` (DREAM keeps only | ||
| the last ``samples`` draws in its buffer). The | ||
| ``Sampler.extend`` helper computes this for you. |
There was a problem hiding this comment.
Should Smapler.extend be doubly backquoted? It was singly backquoted in the original.
There was a problem hiding this comment.
Reverted the prettier changes . . .
…EasyScience/EasyScience into Issue321_EasyList_serialization
…EasyScience/EasyScience into Issue321_EasyList_serialization
…EasyScience/EasyScience into Issue321_EasyList_serialization
|
Will merge despite complaining linter. Sigh, we really need to get this linting updated . . . |
2 participants
This PR fixes the issue described in #321