-
Notifications
You must be signed in to change notification settings - Fork 12
gh-977: Port remaining functions in fields.py
#979
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
Conversation
6555d92 to
d6c3cb6
Compare
d6c3cb6 to
3078c7c
Compare
|
Current regressions seem quite large |
paddyroddy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can you make a comment on the regressions?
|
This PR looks like it will have regressions to should have the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this looks in a good place now. Five things for me:
- Need new line in docstring to match numpydoc
- Move
yieldcomment - Fill in checklist
- Add
need-2-reviewerslabel + Nicolas (if regressions) - Comment the regressions and check if okay
|
Current regressions appear to only be in |
Does this mean the regression is actually coming from the |
It looks like it has been ported but it actually doesn't work with xp = array_namespace(cl)
ell = xp.arange(cl.shape[-1])
return xp.sum((2 * ell + 1) / (4 * xp.pi) * cl, axis=-1)A solution would be the following change. xp = array_namespace(cl)
- ell = xp.arange(cl.shape[-1])
+ ell = xp.arange(cl.shape[-1], dtype=xp.float64)
return xp.sum((2 * ell + 1) / (4 * xp.pi) * cl, axis=-1)Can we make this change? I've raised a PR to fix this - glass-dev/transformcl#6
It could be, however, I think my point of the regressions from the other functions accumulating is still possibly true. Even if the other functions don't fail the regression tests, they could be regressing by 4%. then the total regression of all of them could add up to more than 5%. Or is that not how percentages work? 🤔 |
Yes, I don't see why not
I could buy that argument |
|
Still seems to just be the known |
|
Making an executive decision to accept that this regression is inevitable for now. In the example notebook, the cc: @ntessore |
Description
Ports the functions left in
fields.pywhich haven't yet been portedCloses: #977
Changelog entry
Added: Full support for the array-api to
fields.pyChecks