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

feat: add ndarray/base/map #2715

Merged
merged 66 commits into from
Aug 16, 2024
Merged

Conversation

headlessNode
Copy link
Contributor

Progresses #2656.

Description

What is the purpose of this pull request?

This pull request:

  • Adds a base implementation of Array.prototype.map method for ndarrays

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

This is a work-in-progress pull request.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@headlessNode
Copy link
Contributor Author

@kgryte As discussed, I've implemented the kernels up to 3d and the related files. Please review.

@kgryte kgryte added the Feature Issue or pull request for adding a new feature. label Jul 31, 2024
@kgryte kgryte marked this pull request as draft July 31, 2024 21:36
@kgryte
Copy link
Member

kgryte commented Aug 1, 2024

@headlessNode Thanks for working on this. What you have thus far looks great! Just made some minor edits to ensure the descriptions/conventions match other similar packages. Should be good to continue building this out. 🚀

@headlessNode
Copy link
Contributor Author

@kgryte All kernels implemented. Please review, thanks!

@headlessNode headlessNode marked this pull request as ready for review August 13, 2024 13:21
@kgryte
Copy link
Member

kgryte commented Aug 15, 2024

@headlessNode Am I right in thinking that you'd like a review before we add tests?

@headlessNode
Copy link
Contributor Author

@kgryte Yes. But if you'd prefer, I can implement the tests before your review.

@kgryte
Copy link
Member

kgryte commented Aug 15, 2024

I think you are right to wait, lest we need to make any changes.

@kgryte kgryte added the Needs Review A pull request which needs code review. label Aug 15, 2024
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

LGTM. @headlessNode I think we can go ahead and merge this and then add tests in a follow-up PR. That work for you?

@kgryte kgryte removed the Needs Review A pull request which needs code review. label Aug 16, 2024
@kgryte
Copy link
Member

kgryte commented Aug 16, 2024

Re: the failing benchmarks CI job. I can confirm that the benchmarks run as expected. I ran them locally and all succeeded. Given the number of benchmarks, we're simply at the mercy of GitHub CI time limits.

@headlessNode
Copy link
Contributor Author

@kgryte I have started to work on the tests. I'll open a PR when completed.

I can confirm that the benchmarks run as expected.

Yes, I also checked multiple times on my end ran as expected.

@kgryte
Copy link
Member

kgryte commented Aug 16, 2024

Great! I'll go ahead and merge! 🚀

@kgryte kgryte merged commit 72ed2e1 into stdlib-js:develop Aug 16, 2024
10 of 11 checks passed
@headlessNode
Copy link
Contributor Author

Lots of learning with this one. Thanks for your help @kgryte

@kgryte
Copy link
Member

kgryte commented Aug 16, 2024

Awesome! I know it's a slog, so thanks for persisting!

@headlessNode headlessNode deleted the ndarray-base-map branch August 17, 2024 16:11
gunjjoshi pushed a commit to gunjjoshi/stdlib that referenced this pull request Aug 21, 2024
PR-URL: stdlib-js#2715
Ref: stdlib-js#2656
Co-authored-by: Athan Reines <[email protected]>
Reviewed-by: Athan Reines <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Issue or pull request for adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants