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

Provide a public method for iradon_filters #4952

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hmaarrfk
Copy link
Member

Description

Continuation of #3099

Not sure why
#4950 was failing.

Checklist

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

f = np.zeros(size)
f[0] = 0.25
f[1::2] = -1 / (np.pi * n) ** 2
f[1::2] = -1 / (np.pi * n[1::2])**2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was it a bug which you corrected? I don't understand why f is zero on even values of indices, this does not correspond to Kak and Slaney I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I took a look at one of the previous PRs and apparently it's not the classical ramp filter which is used here. I'll try to understand better which filter is used here. Did you just make this change for the sake of a clearer syntax or is there a difference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at the time, the change was introduced for cleaner syntax.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "fixes" for the filter performance were merged in a pervious PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your current refactoring of n calculation is different from previous one:

>>> size = 10
>>> n1 = np.arange(0, size / 2 + 1, dtype=np.int)
>>> n2 = np.arange(size / 2 - 1, 0, -1, dtype=np.int)
>>> n = np.concatenate((n1, n2))
>>> n[1::2]  # This PR refactoring
array([1, 3, 5, 3, 1])
>>> np.concatenate((np.arange(1, size / 2 + 1, 2, dtype=np.int),
                    np.arange(size / 2 - 1, 0, -2, dtype=np.int)))  # Previous version
array([1, 3, 5, 4, 2])

I don't know which version is correct, but there is a difference...

@jni
Copy link
Member

jni commented Sep 13, 2020

I'm not familiar with these methods but the code looks good to me. I'll let others more familiar with radon review/merge but it'd be good to get this in.

@hmaarrfk could you please elaborate more on the motivation for the PR in the description? When making release notes, it's quite annoying to have to go back 7 links to figure out why this PR was made. (The PR you link to is also described as "continuation of...")

Base automatically changed from master to main February 18, 2021 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants