sorry to touch so many files#77
Conversation
| :return: **x_hat** (np.array[float]) -- integral of dxdt_hat | ||
| """ | ||
| x = scipy.integrate.cumulative_trapezoid(dxdt_hat) | ||
| first_value = x[0] - np.mean(dxdt_hat[0:1]) |
There was a problem hiding this comment.
What is this doing? You have an array dxdt_hat, and you take the slice [0:1], which includes the 0 index and excludes the 1 index. So this is equivalent to just taking dxdt_hat[0]. Then you take the mean of that single entry, which returns that same scalar value again, and then you subtract it off x[0]. But dxdt[0] and x[0] have different units, right? I'm confused. Would it make more sense to just assume the first_value = 0?
| :param dt: | ||
| :return: | ||
| def _x_hat_using_finite_difference(x, dt): | ||
| """Find a smoothed estimate of the true function by taking FD and then integrating with trapezoids |
There was a problem hiding this comment.
Is my interpretation correct here? Why differentiate only to immediately integrate?
|
|
||
|
|
||
| # simple finite difference | ||
| def finite_difference(x, dt): |
There was a problem hiding this comment.
All places FD gets used now import from the main FD code, per #79.
| except ImportError: | ||
| pass | ||
|
|
||
| __friedrichs_kernel__ = utility.__friedrichs_kernel__ |
There was a problem hiding this comment.
There are many places you denote 'private' functions with double underscores on each side. Double underscores are typically for builtin "magic" functions, like basic indexing and math operators. Last I know the convention was prepending a single underscore for privates. Sphinx still gives those a miss, only building docs for the ones that don't start with an underscore. I've been changing the standard to reflect what I think the community is more used to as I go.
I'm also just removing places where you alias a function to its own name. It's easy enough just to call it with utility._whatever later on and save this line.
|
|
||
| def get_filenames(path, contains, does_not_contain=('~', '.pyc')): | ||
| """ | ||
| Create list of files found in given path that contain or do not contain certain strings. |
There was a problem hiding this comment.
We have grep -IR . -e "string" for this, and you're not using this one anywhere, so I think it should go away.
| dt = 0.01 | ||
|
|
||
|
|
||
| class TestLM(TestCase): |
There was a problem hiding this comment.
I didn't even know that pytest can run unittest TestCases, but evidently it can. There's no need to have dependency on both, though. It's simpler and fewer lines to do tests the pytest way.
…This also involved touching all the __init__ files, which led me to discover the convolution code wasn't really in the right place, since it's such a basic utility and used by multiple modules. It now lives in the utilities.
|
|
||
| # make derivative go to zero at ends (optional) | ||
| if options['pad_to_zero_dxdt']: | ||
| if pad_to_zero_dxdt: |
There was a problem hiding this comment.
Why would you need to pad and make the derivative go to zero at the edges? Does that allow this method to be applied to aperiodic functions with any success? In spectral-derivatives, I just tell the user it's only going to work on periodic functions.
|
Thanks for all your work Pavel! |
2 participants
Here's some potential Pavelization. Opening a PR early to give more chance for feedback. Ideally we merge in stages to keep code review manageable and make sure we have buy-in from one another.