search

spencerahill / aospy

Python package for automated analysis and management of gridded climate data
Apache License 2.0
83 stars 12 forks source link

Closes 293 and 299: Better Support for More Data Sets #305

Closed jdossgollin closed 5 years ago

jdossgollin commented 5 years ago

This PR closes #293 and #299 to improve compatability for data sets which follow IRI Data Library (IRIDL) conventions, or for some NCAR-NCEP Reanalysis (and potentially other) data sets.

spencerahill commented 5 years ago

@jdossgollin not sure what happened with your commits that caused this PR to show all of these old changes.

It's generally good practice for each PR to address one single thing; it seems like you tried to combine your existing PRs into one here. But there's no need for that, as they both cover different things.

There was no need to close #296 or #298. Just modify them as necessary with new commits and ping us when each is ready for another round of review. I'll re-open those both and close this one.

Also, less important, but my comment about "Closes ###" refers to the PR message, not title.

Does that all make sense?

spencerahill commented 5 years ago

I was going to re-open them but I see you deleted those branches. Hopefully you didn't delete the branches on your local machine as well. If not, then just resubmit them each as new PRs.

Not sure how this got so complicated. Is there some aspect of branches/commiting/PRs that is confusing or otherwise causing you problems? Thanks for your efforts.

jdossgollin commented 5 years ago

Hmm. Looking at these old commits I suspect that I may have branched from master and not develop. That was definitely a mistake on my end. As for the PR names, I think that makes sense now. Really sorry -- at this point it's obvious that having me do this has caused you two far too much hassle. I think that this actually is coherent as a single PR because it enhances compatibility of aospy of other data sets -- both through the internal grid attributes, and by not assuming that time_weights[BOUNDS_STR] is in its coords. But if you'd prefer I can separate them into two PRs since they touch on different aspects of the code. Given the mess I made of this one, I think it makes sense to delete this branch and resubmit as new PRs -- does that sound reasonable to you? Thanks again,

spencerkclark commented 5 years ago

Looking at these old commits I suspect that I may have branched from master and not develop.

Indeed I suspected the same thing.

Given the mess I made of this one, I think it makes sense to delete this branch and resubmit as new PRs -- does that sound reasonable to you?

No worries @jdossgollin; that sounds perfect. This kind of stuff happens when you're first starting out with Git. I'm sure you'll get the hang of the necessary basics pretty quickly.

spencerahill commented 5 years ago

This kind of stuff happens when you're first starting out with Git. I'm sure you'll get the hang of the necessary basics pretty quickly.

Yes, absolutely. Don't sweat it @jdossgollin.

I think it makes sense to delete this branch and resubmit as new PRs

Agreed, sounds good. Whether one or two doesn't matter so much this time around given that the changes are pretty minor (although the one-overall-thing-per-PR rule remains a good heuristic for future submissions), so feel free to just do one to make it easier.

jdossgollin commented 5 years ago

For future reference for a beginner like me:

I was following the directions at http://xarray.pydata.org/en/stable/contributing.html#pushing-your-changes. First I forked the repo to create jdossgollin/aospy, cloned to my computer, made sure I was in the develop branch, and set the upstream:

git remote add upstream https://github.com/spencerahill/aospy.git
git fetch upstream

Then, to make sure I was up to date,

git fetch upstream
git rebase upstream/master

this was of course the issue -- should have run git rebase upstream/develop. I think that's why it was replaying all the old commits. Perhaps there's a better way to do this, though.

spencerahill commented 5 years ago

Ahh I see: xarray uses master as their day-to-day branch, and stable as their branch for releases. We use develop for day-to-day and master for releases. But that's a very subtle point that newcomers are likely to not catch -- especially when we refer them to xarray's docs on how to contribute! That's our mistake.

It clearly makes sense for us to change this to be more consistent with them. I'll do that as part of the v0.3 release. For now, just replace master with develop in your workflow.

jdossgollin commented 5 years ago

OK @spencerahill while I'm getting a few basic pointers: at what point to I update docs/whats-new.rst? I see all the other bullet points there reference both an issue and a PR by number, but I don't know the number of the PR until I submit it (I can try to find the most recent issue and add one to it...)

spencerahill commented 5 years ago

Create the PR, which assigns it a number, and then push another commit with the what's new entry that uses that number. For this reason, usually the what's new entry part is usually the very last step of any PR.