-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
CFTimeIndex calendar in repr #4092
Merged
spencerkclark
merged 37 commits into
pydata:master
from
aaronspring:AS_CFTimeIndex_repr_calendar
Jul 23, 2020
Merged
Changes from 23 commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
0bd61d9
add property
28cbf84
test repr skip
c5d36cc
repr
8d9ebe0
linting
c1cac9f
remove unnecessary
d09092a
remove unnecessary
59dc912
add quotation marks to calendar
1fccb53
add length to wrapper
a52e997
linting
6d022ea
coords.to_index() if CFTimeIndex
bf8c5a0
to_index() iff CFTimeIndex
1e809d0
revert linting
1bfdc7c
revert linting
d58cb7d
revert linting
a0d00ab
to_index in short_data_repr_html
019d309
refine test and rm prints
269e967
fix to pass all tests
68a37c6
revert linting changes
0907cef
revert to_index()
a9c048f
require cftime for added test
fcb48fd
implement format_array_flat repr without commata and multiple lines
83839ec
reproduce pd.Index repr for CFTimeIndex repr
e3c8c01
reproduce pd.Index repr for CFTimeIndex repr
69d000f
sensitive to display_width
80ca891
rewritte format_cftimeindex_array from template of format_array_flat
3080d81
bugfix
b77a2ee
new approach
1e67f3f
Merge branch 'master' into AS_CFTimeIndex_repr_calendar
aaronspring d8d54ce
docstring
4b62406
Merge branch 'AS_CFTimeIndex_repr_calendar' of https://github.com/aar…
249ae24
attrs spaces fix
ecef05a
rm pandas test, refactor format_attrs and repr test dedent
7c31e3a
rm f lint
683f00c
Pass index to format_attrs instead of attrs dict
spencerkclark 2707409
Update whats-new.rst
spencerkclark b7552b3
Add docstring for new calendar property
spencerkclark e8d85db
Update doc/whats-new.rst
spencerkclark File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
format_array_flat
is a good choice if we want a 2-line repr, but for including more values it might be cleaner to write our own logic, rather than add commas and line breaks afterwards. As you've picked up on, I think the fact that we can treat the length of each element of the repr in a cftime array as a constant simplifies things greatly.This is of course ignoring the potential for five-digit years; however, we already assume we won't see those in at least one other place in xarray (partial string indexing). At some point it might be good address that, but I think for now it's ok to stick with the four-digit year assumption. Particularly here, I think the worst that would happen is that the repr might potentially be a few characters wider than the imposed limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, for now I use
format_array_flat
and insert commata and linebreaks manually. Are you suggestion I should rather write a new function? This function would probably use much of the code offormat_array_flat
.concerning the 5digit years:
xr.cftime_range(start='10000',periods=2)
fails withValueError: no ISO-8601 match for string: 10000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now wrote a new function
format_cftimeindex_array
likeformat_array_flat
. Hope this is what you were hoping to see. Both functions share much of the code especially in the beginning of the function. Should I refactor these shared code parts into a small function that both functions use?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, we use the same string parsing logic in
cftime_range
as in partial datetime string indexing. I was thinking of dates one might read in from a file, which get decoded throughcftime.num2date
. Anyway I acknowledge that is an issue we don't need to address at the moment!Sorry I was thinking something more along these lines for the code that formats the times (the rest of the repr can be added around what it generates):
In other words iteratively generating each row in the repr, inserting the separator as you build each row, and inserting line breaks at the end of each row. I just find it fits in my head better than adding those elements post-hoc. I think you should be able to leverage the code above to construct a "split" repr as well (e.g. one that shows only the first and last 10 elements of the index) by calling
format_times
twice with the appropriate arguments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok. Now I think I get the idea...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the really nice template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implemented thanks to your nice template given above. ready for review @spencerkclark