fix/warnings_sort_QualityEngine.report

[jfsantos-ds]

Core changes

• QualityEngine warnings is now a list
• Changed store_warnings add to append (list method)
• Updated report method, now properly sorts warnings based on priority
• Added warning counts by priority
• Reproduced changes in the DataQuality class

Minor changes

• Updated all engine scripts (changed all engine._warnings.add to engine.store_warning)
• Updated all notebooks, engine.warnings no longer needs to be converted to a list
• Added missing examples of single warning inspection to notebooks
• Fixed LabelInspector engine warning descriptions

[UrbanoFonseca]

we should maintain uniqueness of warnings, otherwise running the same failing test multiple times will generate duplicated warnings which are meaningless. this is solved for the .report method with the set but not for the .warnings property. wdyt of the strategy below?

    def store_warning(self, warning: QualityWarning):
        "Adds a new warning to the internal 'warnings' storage."
        if warning not in self._warnings:
            self._warnings.append(warning)

[jfsantos-ds]

Seems right if this holds true, when we do:

warning not in self._warnings

we are running the warning __eq__ method against each content of _warnings, right? Doing a set of objects also resorts to __eq__ to filter uniques right?

The user might raise the same warning, in the same test but with different parameters. We would not filter in that case using your proposal, right?

Besides that I wonder if self.warnings should be an accessible property and not private. I think we probably should only give access to report and get_warnings methods. ATM get_warnings is not filtering unique warnings but should right on the beginning, just like report. What do you think?

[jfsantos-ds]

Removing warnings as an accessible property would mean switching 3 occurrences of self.warnings to self._warnings and removing the warnings property definition in core engine.
Across the different tutorials the sample warning demo should be changed to engine.get_warnings()[x] too.
Wdyt?

[UrbanoFonseca]

1. I'm not 100% sure of what is the python implementation but from local testing it seems that defining __eq__ is enough to test the presence in arrays (like we are doing in the warning not in self._warnings). From our implementation, we are comparing the {category, test, description, priority} attributes on the __eq__ , so if two warnings of the same test have different parameters, the {category, test} are the same but the at least the {description} should be different (we often add some success/failure metrics to the warning description).
2. Given the necessary sort (by priority) and the optional filtering, we can remove the warnings as a property and keep only the get_warnings method

[jfsantos-ds]

Seems like nothing odd remains.

[UrbanoFonseca]

Nice work! 👍 🚀