BUG: Changed dump(a, F) so it would close file #10055
Conversation
numpy/ma/core.py
Outdated
| with open (F, 'w') as F: | ||
| pickle.dump(a, F) | ||
| else: | ||
| pickle.dump(a, F) |
There was a problem hiding this comment.
Style nit: 4-space indents, please.
There are other functions in this module that need the same fix, if you're going to fix this one.
There was a problem hiding this comment.
Ah, my bad.
I will fix any others I find.
I see now that I should have 'wb' instead of just 'w', as per #5491. Is there a way for me to change this without creating another pull request?
There was a problem hiding this comment.
Leave the wb to a different PR - it's a separate problem, and I don't think we should fix it anyway.
There was a problem hiding this comment.
Is there a way for me to change this without creating another pull request?
I assume you're editing in the github web UI? You can navigate to your branch, and hit the edit button on the files there. It'll create a new commit, but we can squash them all together when you're done.
There was a problem hiding this comment.
Sorry, I didn't see your replies to this comment before I changed the wb. Yes, I'm using the Github web UI - I think I got it to work, thanks!
|
Commit message should start with |
|
@eric-wieser Sorry, I will make sure they start with |
|
No worries, we can correct the prefix when we merge - I just pointed it out since I was expecting you to amend the commit to fixup |
|
As an aside, you should avoid creating PRs from |
|
Oh, I didn't realize I could update this pull request. I can fix Ah, I thought I had to create a personal repo since I can't directly modify the master branch. How else should I do it? |
Also changed `'w'` to `'wb'` in `dump`.
|
You did have to create a personal repo, but you also should have created a branch within that repo. Don't worry about it, you're fine as long as you don't do the same thing again to make another PR before this one is merged. I've backed out the |
|
I think I understand. After forking the repo, I should have created a branch within that repo. But after completing some edits, should I commit to my own master branch and then create a pull request or should I commit to my non-master branch and then create a pull request from that? And should the branch name correspond to the problem I'm fixing? Sounds good. Why were some checks not successful for your commit? All you did was remove some whitespace and change |
They were cancelled because I made a second commit. |
|
You should make the PR from the branch by pushing the branch to your github repo, then making the PR from there. |
|
@orbit-stabilizer You only have one personal fork of numpy, and your fork only has one master branch. So the problem happens if you ever want to submit two different PRs at the same time: how can you do that if they both need to change your fork's master branch? This is tricky because often people don't realize that they'll want to open a second PR at the time when you're opening the first one. Or... what if you submit a PR, and then it gets rejected. Now your master branch doesn't match numpy's master branch, so you have to somehow clean it up before you can make new PRs. So the general advice is to make your changes in your own branch inside your own fork, and then submit a PR from that; that way you won't get caught by surprise by one of these tricky issues later. |
|
Thanks for all of the help! @njsmith Oh, that makes a lot of sense. Then I suppose I should merge my branches with my master branch each time my PR goes through and I should create a new branch for each new issue. |
|
In practice, there're only three things worth doing with your numpy master branch - either:
Thanks for the patch, @orbit-stabilizer! |
|
@orbit-stabilizer The development reference is at https://docs.scipy.org/doc/numpy-1.13.0/dev/gitwash/development_workflow.html. The recommended workflow varies among projects, but that manual also works for scipy. |
|
@eric-wieser Perfect, thank you. |
| with open(F, 'r') as F: | ||
| pickle.load(F) | ||
| else: | ||
| pickle.load(F) |
There was a problem hiding this comment.
This is a regression - there needs to be a return here
BUG: Fix regression in np.ma.load in gh-10055
BUG: Fix regression in np.ma.load in gh-10055
See #10045