Initial QOIs#61
Conversation
| mean_argrelmax_kwargs = mean_peaks.upstreams[0].kwargs | ||
|
|
||
|
|
||
| def max_value(pdf): |
There was a problem hiding this comment.
I believe this is the old commit. The more recent commit has these functions moved to the tools.py file with docstrings.
|
|
||
| pdf_argrelmax_kwargs = pdf_peaks.upstreams[0].kwargs | ||
| mean_argrelmax_kwargs = mean_peaks.upstreams[0].kwargs | ||
|
|
There was a problem hiding this comment.
This seems like a lot of extra lines, can we reduce it down to 1?
|
|
||
| Parameters | ||
| ---------- | ||
| g : ndarray g(r) of the pdf |
There was a problem hiding this comment.
ndarray and the description need to be on different lines eg
g : ndarray
g(r) of the pdf| return inner | ||
|
|
||
|
|
||
| def max_value(g, r): |
There was a problem hiding this comment.
Why does this take in r? It doesn't seem to be used.
| def average_pearson(group, g,r): | ||
| """Computes the average pearson of this PDF with the rest of the group | ||
|
|
||
| Parameters |
There was a problem hiding this comment.
This has too many indents. Parameters should be on the same indentation as the description line.
|
|
||
|
|
||
| def total_counts(g,r): | ||
| def total_counts(g): |
There was a problem hiding this comment.
I'm not certain we need this function if it is only calling len on an array
|
|
||
|
|
||
| def average_pearson(group, g,r): | ||
| def average_pearson(group, g, r): |
There was a problem hiding this comment.
Can we also compute the complete pearson correction? (Maybe as a separate function?)
There was a problem hiding this comment.
I think I may be confused on the stats here, to my knowledge the pearson's coefficients can be used on two different datasets. Is there a separate coefficient to compare a dataset to an entire group of datasets.
There was a problem hiding this comment.
You could return the list of pearson's coefficents for the new pdf and all the prior pdfs
sbillinge
left a comment
sbillinge
left a comment
There was a problem hiding this comment.
please work on your git commit messages. They suck currently
|
|
||
| Parameters | ||
| ---------- | ||
| g : ndarray g(r) of the pdf |
There was a problem hiding this comment.
does this function only work for a PDF?
There was a problem hiding this comment.
The functions should work for any 2 variable data. I assumed the inputs were pdfs because the end goal. Would it be better to make this more general (x,y instead of r,g)?
There was a problem hiding this comment.
I think x, y would be better. You might identify x and y as independent and dependent variables.
|
|
||
|
|
||
| def max_value(g, r): | ||
| """Returns largest value |
There was a problem hiding this comment.
This is not so helpful. Bear in mind what the docstring is for. It is for a user to understand what this function is for and how it should be used. Also, the docstring shouldn't include the function name, which is effectively what your's does.
In other words, why do we (or rather a user) need this function (rather than just typing np.amax())?
3 participants
QoIs implemented: Highest Value, Total Counts, Tallest Peak Height, Tallest Peak R Value ,Oscillation behavior
Oscillation Behavior doesn't quite work yet as a QOI.