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 propagation of badflag with .= (Ops::assgn) for sf.net bug 3543056. #51

Merged
merged 1 commit into from Apr 19, 2019

Conversation

mohawk2
Copy link
Member

@mohawk2 mohawk2 commented Mar 2, 2015

Rebased version of #2. Passes tests.

@zmughal
Copy link
Member

zmughal commented Mar 2, 2015

I believe this needs to only run when WITH_BADVAL=1. I don't think the test does so yet.

@mohawk2
Copy link
Member Author

mohawk2 commented Mar 2, 2015

We'll be able to tell more easily once #18 is merged and we rebase against it.

@mohawk2 mohawk2 force-pushed the hainest-assgn_badflag branch 2 times, most recently from e5b7a5d to 824bc6e Compare March 3, 2015 01:25
@mohawk2
Copy link
Member Author

mohawk2 commented Mar 3, 2015

Temporary close because Travis fouls up with race condition when both branch and PR.

@mohawk2 mohawk2 closed this Mar 3, 2015
@zmughal zmughal reopened this Mar 3, 2015
@zmughal zmughal force-pushed the hainest-assgn_badflag branch 2 times, most recently from 0a29cb6 to 6f8d393 Compare March 3, 2015 17:39
@zmughal
Copy link
Member

zmughal commented Mar 3, 2015

LIke I was saying in IRC http://irclog.perlgeek.de/pdl/2015-03-03#i_10215431, this won't fix the code until the badflag does not need to be set. Turning into TODO tests for now.

@zmughal zmughal added the pr:wip label Mar 4, 2015
@mohawk2 mohawk2 force-pushed the hainest-assgn_badflag branch 2 times, most recently from 2f47a52 to 6b24622 Compare March 15, 2015 21:48
@mohawk2
Copy link
Member Author

mohawk2 commented Mar 21, 2015

@hainest, what are your thoughts on the readiness state of this? Does work remain to be done?

@hainest
Copy link

hainest commented Mar 22, 2015

I think that as long as folks are content with the lack of child->parent bad flag propagation, then this can be pulled into master. The (admittedly verbose) idiom for ensuring bad flag propagation is provided in the comments of assign.

@mohawk2
Copy link
Member Author

mohawk2 commented Mar 22, 2015

I think merging should wait until post 2.008, which is imminent. However, there is talk of a much faster release cycle after that, so that'll help.

May I suggest changing the location of telling people the correct idiom move from comments, to actual documentation? Feel like making a PR onto this branch? I'll then bring it across and we can see how it plays.

@hainest
Copy link

hainest commented Mar 27, 2015

May I suggest changing the location of telling people the correct idiom move from comments, to actual documentation?

I misspoke. The description is in the Doc field, so it should be rendered to the documentation automatically.

Feel like making a PR onto this branch?

I thought it was already pulled. Github says that I can merge the PR, but I'm not sure how that aligns with the rebase procedure.

@mohawk2
Copy link
Member Author

mohawk2 commented Mar 28, 2015

It means this branch, as rebased by me, can be merged. It doesn't mean you can do so, until you get a commit bit on the appropriate repo (of which this is a mirror). If you make your fork be up to date with this repo, then branch off this branch, then PR onto this branch (or against master, I'll figure it out), then you can submit changes to your code here. That is what I am saying you could do, if you feel it useful.

@hainest hainest mentioned this pull request Apr 8, 2015
@mohawk2 mohawk2 force-pushed the hainest-assgn_badflag branch 2 times, most recently from efaeee7 to 22d290a Compare April 11, 2015 04:47
@coveralls
Copy link

coveralls commented Apr 11, 2015

Coverage Status

Coverage remained the same at 50.945% when pulling 1437cd8 on hainest-assgn_badflag into 27884a4 on master.

@wchristian wchristian closed this Mar 1, 2018
@wchristian wchristian deleted the hainest-assgn_badflag branch March 1, 2018 20:35
@wchristian wchristian restored the hainest-assgn_badflag branch March 1, 2018 20:50
@wchristian wchristian deleted the hainest-assgn_badflag branch March 1, 2018 23:00
@wchristian wchristian restored the hainest-assgn_badflag branch March 1, 2018 23:15
@wchristian wchristian restored the hainest-assgn_badflag branch April 4, 2018 20:11
@wchristian wchristian deleted the hainest-assgn_badflag branch April 6, 2018 11:50
@wchristian wchristian restored the hainest-assgn_badflag branch April 6, 2018 11:56
@wchristian wchristian deleted the hainest-assgn_badflag branch April 6, 2018 12:51
@wchristian wchristian restored the hainest-assgn_badflag branch April 6, 2018 12:56
@wchristian wchristian deleted the hainest-assgn_badflag branch April 9, 2018 14:31
@wchristian wchristian restored the hainest-assgn_badflag branch April 9, 2018 14:43
@wchristian wchristian deleted the hainest-assgn_badflag branch April 24, 2018 08:22
@wchristian wchristian restored the hainest-assgn_badflag branch April 24, 2018 08:25
@wchristian wchristian deleted the hainest-assgn_badflag branch May 4, 2018 02:40
@wchristian wchristian restored the hainest-assgn_badflag branch May 4, 2018 02:45
@mohawk2 mohawk2 reopened this May 22, 2018
@PDLPorters PDLPorters deleted a comment from coveralls Apr 14, 2019
@PDLPorters PDLPorters deleted a comment from coveralls Apr 14, 2019
@PDLPorters PDLPorters deleted a comment from coveralls Apr 14, 2019
@PDLPorters PDLPorters deleted a comment from coveralls Apr 14, 2019
@PDLPorters PDLPorters deleted a comment from coveralls Apr 14, 2019
@PDLPorters PDLPorters deleted a comment from coveralls Apr 14, 2019
@PDLPorters PDLPorters deleted a comment from coveralls Apr 14, 2019
@PDLPorters PDLPorters deleted a comment from coveralls Apr 14, 2019
@mohawk2
Copy link
Member Author

mohawk2 commented Apr 14, 2019

Rebased to update for t/ops.t change.

@mohawk2
Copy link
Member Author

mohawk2 commented Apr 16, 2019

@hainest @d-lamb @devel-chm @zmughal @drzowie Is this ready for primetime? :-)

@hainest
Copy link

hainest commented Apr 16, 2019

@mohawk2 I think so. I would reiterate my previous comment:

I think that as long as folks are content with the lack of child->parent bad flag propagation, then this can be pulled into master. The (admittedly verbose) idiom for ensuring bad flag propagation is provided in the doc for assign.

@mohawk2
Copy link
Member Author

mohawk2 commented Apr 19, 2019

Limitations are documented. Tests are passing. Merging.

@mohawk2 mohawk2 merged commit 19563d7 into master Apr 19, 2019
@mohawk2 mohawk2 deleted the hainest-assgn_badflag branch April 19, 2019 18:17
mohawk2 added a commit that referenced this pull request Apr 16, 2022
@mohawk2
Copy link
Member Author

mohawk2 commented Apr 16, 2022

The above-linked commit actually now means assgn does not need the badflag set on its output ndarray, which is nice to finally achieve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants