Skip to content

fix: add missing ndarray test fixture#1054

Merged
kgryte merged 1 commit intostdlib-js:developfrom
rotu:patch-1
Jul 18, 2023
Merged

fix: add missing ndarray test fixture#1054
kgryte merged 1 commit intostdlib-js:developfrom
rotu:patch-1

Conversation

@rotu
Copy link
Copy Markdown
Contributor

@rotu rotu commented Jul 18, 2023

Description

What is the purpose of this pull request?

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.

No.

Checklist

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


@stdlib-js/reviewers

Copy link
Copy Markdown
Contributor

@stdlib-bot stdlib-bot left a comment

Choose a reason for hiding this comment

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

🎉 Welcome! 🎉

And thank you for opening your first pull request! We will review it shortly. 🏃 💨

@rotu rotu marked this pull request as ready for review July 18, 2023 04:09
@kgryte kgryte added the Bug Something isn't working. label Jul 18, 2023
@rotu
Copy link
Copy Markdown
Contributor Author

rotu commented Jul 18, 2023

Backstory: I was looking at using stdlib ndarray but saw the "test: failing" CI badge at https://github.com/stdlib-js/ndarray, which gave me pause. I'm not sure if this is all that's needed to fix the tests.

Also, I copied the code from https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/ndarray/dispatch/test/fixtures/fill.js. It seems to me like maybe fill is a useful operation in itself, not just for test code. But I'm not yet familiar enough with the code.

@kgryte
Copy link
Copy Markdown
Member

kgryte commented Jul 18, 2023

Backstory: I was looking at using stdlib ndarray but saw the "test: failing" CI badge at stdlib-js/ndarray, which gave me pause. I'm not sure if this is all that's needed to fix the tests.

Ah! Unless you need the entire namespace, I suggest using the individual packages. E.g.,

$ npm install @stdlib/ndarray-array

The omission of the fixture file is my fault. I believe I have it on another dev machine. Let me circle back.

It seems to me like maybe fill is a useful operation in itself, not just for test code.

Indeed. The main thing holding this up is implementing the C kernel, which we haven't had the bandwidth to address. For the test fixtures, it's fine if these implementations are not optimal, as we don't care as much about raw perf, but they aren't really appropriate as general packages.

We do have lower-level implementations which do implement kernels (e.g., @stdlib/ndarray/base/unary, where, when the callback is a "constant" function, can be used to fill an output array), but these implementations are arguably a bit heavyweight and not as ergonomic when used in tests.

Copy link
Copy Markdown
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.

I, indeed, had failed to commit this file. The implementation matches the uncommitted file. Will merge in order to resolve failing builds.

@kgryte
Copy link
Copy Markdown
Member

kgryte commented Jul 18, 2023

Thanks, @rotu!

@kgryte kgryte merged commit 2ef370c into stdlib-js:develop Jul 18, 2023
@rotu rotu deleted the patch-1 branch July 19, 2023 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants