Switch from procedural to functional calling style, using returns.#17
Switch from procedural to functional calling style, using returns.#17gsmecher wants to merge 13 commits intosfilip:masterfrom
Conversation
|
I would like to continue along these lines - I am not delving deeply into the mathematics, but would like to make the code shorter and more consistent where I am able. This pull request is a "test particle" - please let me know what you think. I am aiming towards allowing arbitrary amplitudes to be specified in an external-facing API. I see you have special-case treatments for Hilbert and differentiator filters already - I would like to be able to do CIC compensation along similar lines, and I think it makes more sense to expose amplitude specifications to the public API than it does to bury CIC-specific code in your library. |
|
Sure. Go ahead. I'm happy with exposing more useful functionality. I will hold off on reviewing this PR if you want to continue with changes in this branch. |
This simplifies call sites in pm.cpp, and avoids a number of assumptions about arrays being allocated with the correct sizes. I am not a C++ guru, but I believe this change will not produce less performant code using c++17 or later compilers (which guarantee copy elision under these circumstances.)
For example, uniform(), chebvand(), wam(). As with the previous commit, these calling styles reduce code overhead and avoid the need to pre-allocate vectors/matrices with a particular size (which sometimes matters, and sometimes doesn't.) I think compilers that do NRVO (often mandatory under c++17) should produce code that avoids copies and is just as performant.
gsmecher
force-pushed
the
functional_calling_style
branch
from
b935b27 to
86daf46
Compare
…wam(). Aside from performance (which is probably not worth getting worked up about here), cosntness helps clarify when references are used as pure inputs and when they are modified along the way.
Several other calls in cheby.cpp have side-effects and can't be const-ified without internal changes.
|
Great - thanks. These commits (so far) are largely janitorial - they make sense on their own, but are also possible intrusions into your coding style. I'll comment here when I'm ready for a closer look. And - of course, thanks for your efforts in researching and creating this code and releasing it permissively. |
Note that the use of structured bindings (in compdelta) requires a shift to c++17. An alternative would be to use std::tie - it's a trivial change and I don't know how broadly you intend compiler compatibility.
This conversion direction is neither used nor independently tested.
The degree check above this if statement will always guarantee we're using AFP when fband count > (degree+2)/4.
|
Question on pm::uniform, which may be a more general question. There are (I think) no "point bands" in the test suite, so the code in pm::uniform() that detects and responds to bands with bandwidth 0 is not exercised. If this is dead code, I'll push a commit that removes it. On the other hand, if you have plans for this code (or if it plays a role I've missed) you may prefer to leave it alone. (Comparisons to 0.0 in a floating-point regime are, of course, very brittle - this code will likely not be robust unless the 0-width bands in question are directly passed in as a user specification. I don't know what the use-case for point bands is in either case.) This is similar to the backwards conversion code in bandconv(), which never actually exercised the TOFREQ direction. There is a commit above (43df49e) that removes this code path. Before I spend too much time tracking down similar code reductions, I thought I'd gauge your reaction here. Again, I'm working towards useful changes that are externally visible - but this kind of code reduction is an exercise on the way. |
|
Regarding the "point bands" code, this is something that I added in as it is also supported by the Matlab implementation of firpm. I admit that this is the only reason why it is there. No direct use case pops to mind right now. Despite this, I am not sure I want it removed. The fact that it is allowed in the Matlab implementation is enough reason for me to keep it in. I know that comparisons to 0.0 in floating point are in general not a wise decision, but here I am really expecting that the 0-width bands are given directly by the user. The TOFREQ direction was added in for debugging purposes and it does not really serve a purpose now/anymore. It is OK to remove it. My two word reaction is, reduce away! Overall, I am very happy with any simplifications you can bring to the code and exposing any functionality that might be interesting for the user. Thank you again for the interest that you've taken in this project and all the work that you are doing to improve it! |
|
I am not familiar with the Matlab FIRPM implementation (it has been years since I used it) but I see an example here: https://www.mathworks.com/help/dsp/ug/minimax-fir-filter-design.html ...under the heading "Filter Designed for Specific Single-Point Bands". It looks like this can be used e.g. to enforce a zero at a specific point in frequency-space, and I can imagine where it might be useful (e.g. notching harmonics of 60 Hz in a PFC controller?) Thanks for the thoughtful response. If a specific test case comes up, I'll add it to the regression tests - otherwise, I'll leave it alone. best, |
2 participants
This simplifies call sites in pm.cpp, and avoids a number of assumptions
about arrays being allocated with the correct sizes.
I am not a C++ guru, but I believe this change will not produce less
performant code using c++17 or later compilers (which guarantee copy
elision under these circumstances.)