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

CFtime bug in gridmap #68

Merged
merged 6 commits into from
Apr 24, 2023
Merged

Conversation

Beauprel
Copy link
Contributor

@juliettelavoie can you check if this fix works, or send me a path to a similar file?

@Beauprel Beauprel linked an issue Apr 20, 2023 that may be closed by this pull request
@Beauprel Beauprel marked this pull request as ready for review April 24, 2023 15:53
@Beauprel Beauprel requested a review from juliettelavoie April 24, 2023 15:57
for name, obj in xr_objs.items():
if "time" in obj.dims:
if isinstance(obj.get_index("time"), xr.CFTimeIndex):
conv_obj = obj.convert_calendar("standard", use_cftime=None)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function might be a bit dangerous depending on what we are plotting... If we want to see the no leap or 360_day on the time axis, this might be problematic. At least, add a warning to say that you are converting the calendar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. But I see this as a separate issue - check_timeindex is not used in gridmap.

Copy link
Contributor Author

@Beauprel Beauprel Apr 24, 2023

Choose a reason for hiding this comment

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

Ah, ok I see. I had initially modified this function to accept DataArrays and Datasets, and not just dictionaries, to be able to use it in gridmap. This didn't change the logic of how we convert calendars. My latest commit removed it from gridmap.

I'll add a warning for now, and create a separate issue (#72), as I need to look into why I was converting the calendar in the first place.

Copy link
Contributor

@juliettelavoie juliettelavoie left a comment

Choose a reason for hiding this comment

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

chnage the warning and then it's good to go

@Beauprel Beauprel merged commit 801badd into main Apr 24, 2023
@Beauprel Beauprel deleted the 61-show_time-doesnt-work-with-cftime branch April 24, 2023 18:24
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.

show_time doesn't work with cftime
2 participants