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

Adds support for np.datetime64 and strings with timezone info #1344

Closed
wants to merge 14 commits into from

Conversation

ankitkmr
Copy link
Contributor

This offers a solution to the issue #798

@ankitkmr
Copy link
Contributor Author

@dpshelio Finally added support for np.datetime64 input which I guess the issue was about in the first place. Also added support for input strings with timezone information. Added (some important to this issue) Test Cases that are supported. Also solved the problem of possible multiple '-' in time_string

@dpshelio
Copy link
Member

@ankitkmr why did you closed the other PR? You could have just updated it... Well, my comments from #1343 still apply here, e.g., there is repetition in the code that should be reduced.
The tests have to go in the tests files and using pytest. Check SunPy's dev documentation for more details on how to do it. And please, update the pull request by updating the branch where you are doing the changes, not closing and opening a new one each time.

@ankitkmr
Copy link
Contributor Author

@dpshelio Umm sorry I couldn't figure out back then that I could simply commit and update that PR in itself. It wont happen in future. As for the repetition, corrected.
And yeah I'll do the test cases sorry didn't know that thats how its supposed to be done !! in the meanwhile I made up the blog let me know if its ok !!

http://home.iitk.ac.in/~ankitkmr/blog/

@ankitkmr
Copy link
Contributor Author

@dpshelio OK I condensed it even more I agree too much useless code there !! Oops..I added the tests as well according to the dev guide. They can be found in PR1345 (new PR) this I guess happened because I was committing to a new file.

@dpshelio
Copy link
Member

No, they are not in a different pul request because you've made changes in different files, but because you made the changes in a different branch. If you do it in the same branch you would see it here.
I would suggest you to close the new PR #1345 and cherrypick the commit you did into this branch ankitkmr-patch-3

So, probably you could do (I've not tested it):

$ git checkout ankitkmr-patch-3
$ git cherry-pick 1821096
$ git push 

@ankitkmr
Copy link
Contributor Author

@dpshelio Done !! Now I think I am moving on to writing my proposal !!

@wafels
Copy link
Member

wafels commented Mar 23, 2015

As written, the code requires timedelta to be imported explicitly in order for this PR to pass.

@ankitkmr
Copy link
Contributor Author

Thanks @wafels just noticed that and added its import to the test_time.py file

@Cadair Cadair added the Needs Adoption PRs that were abandoned but should be picked up again and merged in label Nov 4, 2015
@Cadair Cadair closed this Jan 5, 2017
@nabobalis nabobalis added the time Affects the time submodule label Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Adoption PRs that were abandoned but should be picked up again and merged in time Affects the time submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants