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

Add fftfreq (open question(s)) #950

Merged
merged 8 commits into from
Dec 21, 2022
Merged

Add fftfreq (open question(s)) #950

merged 8 commits into from
Dec 21, 2022

Conversation

skeydan
Copy link
Collaborator

@skeydan skeydan commented Dec 19, 2022

Hi @dfalbel, here's my next try at fftfreq() :-).

I still have two questions (one of which is a problem, the other one just something to clarify at this point).

The problem: I see that I'm handling the out tensor incorrectly (this is evident from the test in wrapers.R already).
I was looking for examples for how to do this in the code base, but didn't find anything that matched closely enough, so what I did was following what is seen in test-storage.R ... Probably this could not even work in this case, since here, a function call is involved ...
In reality, I do not even see why that option would be useful (to be able to pass in a tensor here), so if you like we could also just remove that argument ... (I would still be interested in knowing whether it'd be feasible in principle).

The question: How does gen-namespace-examples.R work? I see it used also for function which, in gen-namespace-docs.R, already have examples? I guess I don't need to add anything there, but if I did, how'd I generate the hash?

Thanks!!

@skeydan
Copy link
Collaborator Author

skeydan commented Dec 19, 2022

Oh wait ... (re "problem") ... started looking at another PR-to-be (for torch_where) and realized that what I need is a complementing _out function! And that exists already, and as far as I see, I can just leave it as-is ... since I wouldn't know why someone would want to export it! Right?

Well in fact there still is one thing I'll modify there: the d argument should not have a default of 1L, since it's a float (nothing to do with dimensions) ... I think it should be fine to make that change inside this PR, since I'm making the analogous modification in fftfreq ...

Going to update the PR, please let me know if you disagree :-)

@skeydan
Copy link
Collaborator Author

skeydan commented Dec 19, 2022

Me again... I think what I proposed above isn't the correct way to handle the 1L ... (shouldn't update gen-namespace.R manually anyway) ... I see this in r.R:

if (default %in% c("1", "0", "-1", "2", "-2", "100", "-100",
                     "20")) {
    return(paste0(default, "L"))
  }

What do we do about this? I'll leave it untouched for now, and just remove the case-handling code for out as I said before.

@skeydan
Copy link
Collaborator Author

skeydan commented Dec 19, 2022

OK, I think that should do it ...

Copy link
Member

@dfalbel dfalbel left a comment

Choose a reason for hiding this comment

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

Looks great! Just minor updates in the docs.

R/gen-namespace-docs.R Outdated Show resolved Hide resolved
R/gen-namespace-docs.R Outdated Show resolved Hide resolved
R/gen-namespace-docs.R Outdated Show resolved Hide resolved
@dfalbel
Copy link
Member

dfalbel commented Dec 20, 2022

Regarding the defaults: since you already use the correct default in the wrapper function there's no need to modify the code generation or the generated function as it will never be called with its own default.

Regarding out: I think most of this out arguments are used for saving memory, by writing the results of a computation in an unused buffer, but we don't currently support those in R and if add support for them in the future it will probably be made in the C/C++ level instead.

Regarding examples: There's some functionality in here that was used to generate the docs but I would be surprised if it still works. It has been a long time I don't try it - it was mostly used to generate the first batch of functions.

skeydan and others added 3 commits December 20, 2022 21:39
Co-authored-by: Daniel Falbel <[email protected]>
Co-authored-by: Daniel Falbel <[email protected]>
Co-authored-by: Daniel Falbel <[email protected]>
@skeydan
Copy link
Collaborator Author

skeydan commented Dec 20, 2022

Many thanks for the review and the explanations, - got it!

@dfalbel dfalbel merged commit 9b8c46c into main Dec 21, 2022
@dfalbel dfalbel deleted the fftfreq2 branch December 21, 2022 12:01
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.

2 participants