DM-38386: Add utility function to calculate safe plotting limits #151
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
==========================================
+ Coverage 92.42% 93.61% +1.19%
==========================================
Files 41 43 +2
Lines 2654 2680 +26
==========================================
+ Hits 2453 2509 +56
+ Misses 201 171 -30
... and 20 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
python/lsst/utils/plotting/limits.py
Outdated
| import numpy as np | ||
|
|
||
|
|
||
| def calculateSafePlottingLimits(dataSeries, percentile=99.9, constantExtra=None, symmetricAroundZero=False): |
There was a problem hiding this comment.
| def calculateSafePlottingLimits(dataSeries, percentile=99.9, constantExtra=None, symmetricAroundZero=False): | |
| def calculateSafePlottingLimits(dataSeries, percentile: float = 99.9, constantExtra: Optional[float] = None, symmetricAroundZero: bool = False) -> tuple[float, float]: |
There was a problem hiding this comment.
You may also need from __future__ import annotations at the top since we still use python 3.8.
There was a problem hiding this comment.
Also will need to import Optional from typing up top. I'm not really sure what dataSeries is an iterable of. Floats? So maybe Union[Iterable[float], Iterable[Iterable[float]] ?
There was a problem hiding this comment.
Instead of Optional, use float | None and avoid that import. And if dataSeries is effectively "numpy array-like", then typing.Any may be our best bet. Others may have had better (and more extensive) experiences, but I'm pretty skeptical of the value of typing anything numpy-related.
Edit: ah, if we're still trying to support Python 3.8, then float | None is out. I'm guessing we're only stil trying to on this ticket so Merlin doesn't have to be the one to edit all the package metadata right now?
There was a problem hiding this comment.
except we still support python 3.8 and I don't know if that works there.
|
This PR is going to need a test file. I note that this is not a plotting function at all but a statistics function, so is plotting the right namespace for it? Also this is a new function in utils so should use snake case. |
mfisherlevine
force-pushed
the
tickets/DM-38386
branch
from
a5824e7
to
4302e1b
Compare
mfisherlevine
force-pushed
the
tickets/DM-38386
branch
from
ff4d63b
to
1a6598d
Compare
mfisherlevine
force-pushed
the
tickets/DM-38386
branch
from
c6edfc4
to
3203014
Compare
python/lsst/utils/plotting/limits.py
Outdated
| The value to set the ylim maximum to. | ||
| """ | ||
| if not isinstance(data_series, Iterable): | ||
| raise ValueError("data_series must be either an iterable, or an iterable of iterables") |
There was a problem hiding this comment.
Please add a quick test that this ValueError happens if you pass in something like a single float.
There was a problem hiding this comment.
Done, and changed it to a TypeError too, because, well, it is 🙂
| # now we're sure we have an iterable, if it's just one make it a list of it | ||
| # lsst.utils.ensure_iterable is not suitable here as we already have one, | ||
| # we would need ensure_iterable_of_iterables here | ||
| if not isinstance(data_series[0], Iterable): # np.array are Iterable but not Sequence so isinstance that |
There was a problem hiding this comment.
I am concerned that we are being a bit inconsistent. Remind me what the type error was if Iterable is used? numpy not being a Sequence is a bit odd.
There was a problem hiding this comment.
Yes, I hate hate hate this. Getting this to work with a list, a numpy array, and a column from a pandas.dataframe, and an iterable of all of these was not super fun. This was the best solution I found. It is quite likely someone who knows what they're doing with mypy could do better, but I could not.
There was a problem hiding this comment.
I am absolutely not suggesting a change to this particular code (especially now that it's working and merged), but in general when you run into trouble making a whole host of differently-behaving argument types work in single function, it might be time to make it into multiple functions that share code. Python was not designed to support function overloading, and while we often want it to avoid having to come up with or export multiple names, sometimes that's the better price to pay.
Checklist
doc/changes