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 array/base/mskbinary4d #3196

Merged
merged 69 commits into from
Nov 25, 2024
Merged

Conversation

AbhijitRaut04
Copy link
Contributor

Resolves #3179 .

Description

What is the purpose of this pull request?

This pull request:

  • implements the mskbinary4d package for masked binary operations in the array/base namespace.
  • and provides a 4D equivalent of the mskbinary4d package for binary callbacks using a mask array.

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.

No.

Checklist

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


@stdlib-js/reviewers

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.

This is looking good. Thanks for working on this. Once the initial nitpicks have been resolved, I'll do a closer final review.

@kgryte
Copy link
Member

kgryte commented Nov 20, 2024

One thing that needs to be resolved before further review is the failing unit tests.

@kgryte
Copy link
Member

kgryte commented Nov 20, 2024

@AbhijitRaut04 One last thing, you'll want to make sure you are branching off of develop. Based on the commit history above, that does not appear to be the case. Your dev flow should be

$ git checkout develop
$ git pull upstream develop
$ git checkout -b mskbinary4d

Do work. Open a PR. Get it merged.

Then, to work on a new feature, repeat the above.

$ git checkout develop
$ git pull upstream develop
$ git checkout -b feat/new_feature

where upstream has been configured to be

$ git remote add upstream https://github.com/stdlib-js/stdlib.git

@kgryte kgryte added Feature Issue or pull request for adding a new feature. Needs Changes Pull request which needs changes before being merged. labels Nov 20, 2024
@AbhijitRaut04 AbhijitRaut04 requested a review from kgryte November 20, 2024 08:37
@AbhijitRaut04
Copy link
Contributor Author

@AbhijitRaut04 One last thing, you'll want to make sure you are branching off of develop. Based on the commit history above, that does not appear to be the case. Your dev flow should be

$ git checkout develop
$ git pull upstream develop
$ git checkout -b mskbinary4d

Do work. Open a PR. Get it merged.

Then, to work on a new feature, repeat the above.

$ git checkout develop
$ git pull upstream develop
$ git checkout -b feat/new_feature

where upstream has been configured to be

$ git remote add upstream https://github.com/stdlib-js/stdlib.git

@kgryte I have now discarded the commits of develop branch which i have raised. Will it work ?

@kgryte
Copy link
Member

kgryte commented Nov 20, 2024

I have now discarded the commits of develop branch which i have raised. Will it work ?

My comment was intended to apply to all future PRs. For the current PR, you should leave the Git history, as is. The current diff is fine.

@AbhijitRaut04
Copy link
Contributor Author

I have now discarded the commits of develop branch which i have raised. Will it work ?

My comment was intended to apply to all future PRs. For the current PR, you should leave the Git history, as is. The current diff is fine.
I have raised a new pr for issue #3180 after running above commands but the history is not refining.

@kgryte
Copy link
Member

kgryte commented Nov 20, 2024

I have raised a new pr for issue #3180 after running above commands but the history is not refining.

Likely because you have messed up your local develop commit history. I suggest holding off on any further PRs. Prioritize getting your current set of PRs merged. Once those PRs are merged, then you should delete your local fork and start over by cloning the repository and from then on follow the procedure outlined above.

@AbhijitRaut04
Copy link
Contributor Author

I have raised a new pr for issue #3180 after running above commands but the history is not refining.

Likely because you have messed up your local develop commit history. I suggest holding off on any further PRs. Prioritize getting your current set of PRs merged. Once those PRs are merged, then you should delete your local fork and start over by cloning the repository and from then on follow the procedure outlined above.

Ok I will wait to for it. I have raised pr's for issues #3180 #3179 #1942 and #2106 please take a look to it. I am ready to contribute.

@stdlib-bot
Copy link
Contributor

Coverage Report

Package Statements Branches Functions Lines
array/base/mskbinary4d $\color{green}171/171$
$\color{green}+100.00\%$
$\color{green}11/11$
$\color{green}+100.00\%$
$\color{green}1/1$
$\color{green}+100.00\%$
$\color{green}171/171$
$\color{green}+100.00\%$

The above coverage report was generated for the changes in this PR.

@AbhijitRaut04
Copy link
Contributor Author

AbhijitRaut04 commented Nov 21, 2024

@kgryte I have applied the changes suggested. I kindly request you to review this PR.

@kgryte kgryte changed the title feat: add implementation for array/base/mskbinary4d feat: add array/base/mskbinary4d Nov 25, 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. Thanks, @AbhijitRaut04!

@kgryte kgryte added Utilities Issue or pull request concerning general utilities. and removed Needs Changes Pull request which needs changes before being merged. labels Nov 25, 2024
@kgryte kgryte merged commit cb27ec4 into stdlib-js:develop Nov 25, 2024
19 checks passed
@AbhijitRaut04 AbhijitRaut04 deleted the mskbinary4d branch November 25, 2024 07:29
@AbhijitRaut04 AbhijitRaut04 restored the mskbinary4d branch November 25, 2024 07:31
@AbhijitRaut04 AbhijitRaut04 deleted the mskbinary4d branch November 25, 2024 07:31
pranav-1720 pushed a commit to pranav-1720/stdlib that referenced this pull request Nov 25, 2024
PR-URL: stdlib-js#3196
Closes: stdlib-js#3179
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. Utilities Issue or pull request concerning general utilities.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: add array/base/mskbinary4d
4 participants