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

Switch logic for using all-data in particle IO #2391

Merged
merged 1 commit into from
Dec 16, 2019

Conversation

matthewturk
Copy link
Member

After examining #2383, I came to believe that this logic was inverted.
In short, I believe that if is_all_data is True, we should be using
all of the data without validating. If it is False, we should check
for collisions.

@ngoldbaum
Copy link
Member

Oops, my fault. Thanks for the fix!

It looks like this pointing at master instead of yt-4.0.

@matthewturk matthewturk changed the base branch from master to yt-4.0 December 13, 2019 18:42
@matthewturk
Copy link
Member Author

Whoops, you're right! I just changed the base and I think it's correctly pointed now.

Copy link
Member

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

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

This looks correct to me, thanks for fixing my bug!

It looks like github didn't re-trigger the tests when you changed the base, might need to either force-push a no-op amend commit or close and re-open the PR to get them to run on the correct branch.

@matthewturk matthewturk reopened this Dec 13, 2019
@matthewturk
Copy link
Member Author

@yt-fido please test this

@ngoldbaum
Copy link
Member

I don’t think those can be cleared without udjusting the jenkins webhook since jenkins isn’t configured to run on this branch. Regardless this should be good to merge.

@brittonsmith
Copy link
Member

@matthewturk assures me this is ok to merge, so I'm doing it.

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

Successfully merging this pull request may close these issues.

3 participants