Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

review comments: general #1

Closed
grlee77 opened this issue Feb 6, 2020 · 2 comments
Closed

review comments: general #1

grlee77 opened this issue Feb 6, 2020 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@grlee77
Copy link

grlee77 commented Feb 6, 2020

Hi @OverLordGoldDragon

I haven't gotten a chance to look at this in much detail yet, but here are some initial overall comments regarding the code.

General comments

I wouldn't make refactoring the docstrings a first priority though (The same applies for PEP8 issues)

  • 3.) It would be good to make the argument handling more Pythonic.
    For example, rather than specify opts=None or opts={} and then parse it with a _get_opts helper, I would explicitly make the signature of synsq_cwt_fwd
def synsq_cwt_fwd(x, t=None, fs=None, nv=32, type='morlet', difftype='direct', gamma=None)```

so that it is easy for the user to determine the names and defaults of all options.

The same applies to the various other functions such as synsq_squeeze that are called within. It is better to explicitly list the arguments by name than hiding them within an opts dictionary.

@bluenote10
Copy link

For example, rather than specify opts=None or opts={} and then parse it with a _get_opts helper, I would explicitly make the signature of synsq_cwt_fwd

That would be very helpful indeed, and much more idiomatic. Note that passing a bunch of parameters around internally can still be done in a grouped fashion by putting them in a dict kwargs = dict(type=type, difftype=difftype, ...) which is then passed around via **kwargs.

@OverLordGoldDragon
Copy link
Owner

@bluenote10 Agreed, changed my mind since last time; opts's getting ditched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants