feat: adds residual vs fitted plotting function #100
Conversation
fix: corrects typo in docstring
Codecov Report
@@ Coverage Diff @@
## master #100 +/- ##
=======================================
Coverage 96.69% 96.69%
=======================================
Files 12 12
Lines 787 787
=======================================
Hits 761 761
Misses 26 26
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks, @byooooo .
The only thing I'd recommend to change is that we indent the "Defaults to" in the docstrings.
pyepal/plotting/__init__.py
Outdated
| Defaults to "objective [index]". | ||
| figsize (tuple, optional): Figure size for each individual residual | ||
| vs fitted objective plot. | ||
| Defaults to (6.0, 4.0). |
There was a problem hiding this comment.
I feel there should be an indent for "Defaults to"
pyepal/plotting/__init__.py
Outdated
| labels (Union[List[str], None], optional): [description]. Defaults to None. | ||
| figsize (tuple, optional): [description]. Defaults to (8.0, 6.0). | ||
| labels (Union[List[str], None], optional): Labels for each objective. | ||
| Defaults to "objective [index]". |
There was a problem hiding this comment.
probably also needs an indent here
|
|
||
| Returns: | ||
| fig | ||
| """ | ||
|
|
||
| num_targets = y.shape[1] | ||
| fig, ax = plt.subplots( # pylint:disable=invalid-name | ||
| num_targets, num_targets, figsize=figsize | ||
| num_targets, num_targets, figsize=figsize, tight_layout=True |
There was a problem hiding this comment.
something we do not have to fix here but In the function above the figsize argument behaves differently
There was a problem hiding this comment.
yea i thought it made more sense to set the figsize individually for that function and have it auto-adjust by multiplying it based on the number of objectives. probably not good if we want to be consistent with the other function. it didn't make sense to do that for this function since the figure size for each individual subplot will be very small
There was a problem hiding this comment.
ok, we can maybe describe these behaviors in the docstrings
There was a problem hiding this comment.
ah it is already there, sorry!
| @@ -166,6 +166,63 @@ def plot_histogram(y: np.ndarray, palinstance, ax): # pylint:disable=invalid-na | |||
| ) | |||
|
|
|||
|
|
|||
| def plot_res( # pylint:disable=invalid-name | |||
| y: np.array, | |||
| palinstance, | |||
There was a problem hiding this comment.
also not important for this PR, but we might want to do this in a future enhancement: We should also annotate the other types in the functions
|
Probably we can add in another PR simple tests for the plotting functions. This is, we won't check if the plots look correct but at least make sure that no error is thrown if we use the functions (might be relevant if we change some of the PALBase API) |
|
yea actually i was wondering where the unit tests were for this. If you want, i can add it as well |
i didn't write test as i wasn't sure what is the best way to test plotting functions. But we might add some that just check that a non-empty figure is returned with correct number of subplots ... but we I can make a separate PR for that later today |
Types of changes
addresses #72
Actions (for code changes)