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

CompoundMultiIndex in the case of AllowMissing #95 #115

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Novicei
Copy link

@Novicei Novicei commented Dec 5, 2021

I think that in the case of AllowMissing, there is a certain problem with the processing logic of CompoundMultiIndex. It's probably where I modified it.

@Novicei
Copy link
Author

Novicei commented Dec 5, 2021

I think that only in the case of AllowMissing, the case when depth == len(builder)-1 will be added to the output. If this is not the case, it should be added to the output when depth==len(c.Indexs).

@Amier3
Copy link

Amier3 commented Jan 25, 2022

Hey @Novicei

Thanks for raising this PR 😄! I see where you attempted to fix #95 , but it looks like there's a lot more code in the commit that's unrelated to the CompoundMultiIndex issue. Could you submit a clean PR with just the changes related to that issue?

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@Novicei
Copy link
Author

Novicei commented Mar 14, 2022

Hey @Novicei

Thanks for raising this PR 😄! I see where you attempted to fix #95 , but it looks like there's a lot more code in the commit that's unrelated to the CompoundMultiIndex issue. Could you submit a clean PR with just the changes related to that issue?

Sorry, I've been very busy lately, it may take a while

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.

4 participants