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

Decreasing number of calls BaseColumn._array_finalize in MaskedColumn #8806

Closed

Conversation

geekypathak21
Copy link
Contributor

Fix: #6721
I managed upto this

In [2]: c = MaskedColumn([1,2], format='%d')
Finished array finalize (with copy attrs)

In [3]: c[:]
Out[3]: 
<MaskedColumn dtype='int64' format='%d' length=2>
1
2

But there are some errors like:

AttributeError: 'MaskedColumn' object has no attribute '_name'

Because of changing sub-class order. So, should I define these attribute inside MaskedColumn
please suggest

@pllim
Copy link
Member

pllim commented Jun 7, 2019

Failures are real.

@bsipocz bsipocz added this to the v4.0 milestone Jun 7, 2019
@geekypathak21
Copy link
Contributor Author

Failures are real.

Yes, I was not able to figure out. What to do next? I need some help here

@pllim pllim requested a review from mhvk June 8, 2019 14:19
@taldcroft
Copy link
Member

@himanshupathak21061998 - thanks for looking into this tricky issue. For me, I cannot offer much helpful advice without just digging into the problem deeply enough to probably solve it myself. So I could review a working solution, but you may need to get there yourself unless @mhvk can jump in.

@mhvk
Copy link
Contributor

mhvk commented Jun 10, 2019

@himanshupathak21061998 - this is tricky business. My main suggestion would be to try to do just one thing (e.g., the override of data) and see if for that the tests pass.

@geekypathak21
Copy link
Contributor Author

Thanks, @mhvk I would use this approach

@mhvk
Copy link
Contributor

mhvk commented Jun 15, 2019

@himanshupathak21061998 - I was reminded of this because another PR (#6720), which needs it, is threatened with closure. When I looked at it, though, I realized that my original suggestion was unnecessarily complicated. Since this still was missing tests, I felt it would be easiest to just implement the change myself - see #8855.

I think it is simpler so will close this PR. But not without thanking you for taking it on! While not directly used, your effort definitely spurred us on to finally solve this problem. Thanks again!

@mhvk mhvk closed this Jun 15, 2019
@geekypathak21
Copy link
Contributor Author

Ok, I was just trying to solve but now I should try any other issue thanks for helping. I learned a lot because of this issue.

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

Successfully merging this pull request may close these issues.

How many times does MaskedColumn need to be finalized?
5 participants