xCDAT / xcdat

An extension of xarray for climate data analysis on structured grids.
https://xcdat.readthedocs.io/en/latest/
Apache License 2.0
118 stars 12 forks source link

[Bug]: Potential bug in temporal.py module #695

Closed zhangshixuan1987 closed 1 month ago

zhangshixuan1987 commented 1 month ago

What happened?

I recently used the PCMDI metrics package to process the diagnostics for the piControl simulations from the E3SM model. As the model time for the piControl starts from year 1, issues arises when the PCMDI calls the function _drop_incomplete_djf () in "xcdat/temporal.py", specifically, I found the error are pointed to line 1087:

incomplete_seasons = (f"{start_year}-01", f"{start_year}-02", f"{end_year}-12")

The problem to my understanding here is that the start_year and end_year in the piControl simulations (e.g. year 1 to year 50) are not 4-digit format, then the _drop_incomplete_djf () complains in my case that:

warning: failed for v3-SORRM 0051 no ISO-8601 or cftime-string-like match for string: 1-01

I think the function actually expect the "0001-01" rather than "1-01", which caused the problems for my case.

I think that it would be safer to adjust the 1087 in "xcdat/temporal.py" to

incomplete_seasons = (f"{start_year:04d}-01", f"{start_year:04d}-02", f"{end_year:04d}-12") In this way, the 4-digt year will be guaranteed, and my test simulation can pass the above error for piControl simulations.

I reported this use case here to see if this would be an indicator for some code improvements.

What did you expect to happen? Are there are possible answers you came across?

No response

Minimal Complete Verifiable Example (MVCE)

No response

Relevant log output

No response

Anything else we need to know?

No response

Environment

The xCDAT is part of library for PCMDI metrics (https://github.com/PCMDI/pcmdi_metrics). In my case, xcdat version 0.7.1 was complied with the python 3.10 and used as the library for the mcmdi metrics.

tomvothecoder commented 1 month ago

Hi @zhangshixuan1987, thank you for reporting this issue. @lee1043 opened up PR #696 to fix this. We plan on releasing a new version of xcdat (v0.7.2) in the next few weeks that should include this fix.

lee1043 commented 1 month ago

@zhangshixuan1987 I just merged #696 so it is in the main branch now. In the meantime if you like to have the updated functionality in your workflow as early adapter and beta tester, you can clone xcdat github repository to your local, activate your conda env, and manually install xcdat by "pip install ." from your local xcdat directory. This can expedite your test for piControl. Please let us know whether the updated code works okay for you.

zhangshixuan1987 commented 1 month ago

Hi Jiwoo,

Thank you for my suggested fixing! I will update my local library accordingly.

With best wishes,

Shixuan

Shixuan Zhang| Pacific Northwest National Laboratory Earth Scientist, Atmospheric, Climate, & Earth Sciences Division p. 509.375.3926 www.pnnl.govhttps://www.pnnl.gov/science/staff/staff_info.asp?staff_num=9376

From: Jiwoo Lee @.> Date: Monday, September 23, 2024 at 11:42 To: xCDAT/xcdat @.> Cc: Zhang, Shixuan @.>, Mention @.> Subject: Re: [xCDAT/xcdat] [Bug]: Potential bug in temporal.py module (Issue #695) Check twice before you click! This email originated from outside PNNL.

@zhangshixuan1987https://github.com/zhangshixuan1987 I just merged #696https://github.com/xCDAT/xcdat/pull/696 so it is in the main branch now. In the meantime if you like to have the updated functionality in your workflow as early adapter and beta tester, you can clone xcdat github repository to your local, activate your conda env, and manually install xcdat by "pip install ." from your local xcdat directory. This can expedite your test for piControl. Please let us know whether the updated code works okay for you.

— Reply to this email directly, view it on GitHubhttps://github.com/xCDAT/xcdat/issues/695#issuecomment-2369083707, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AIAWVFV5G2OAD2NZRJQDUP3ZYBOJLAVCNFSM6AAAAABOTAWPFKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRZGA4DGNZQG4. You are receiving this because you were mentioned.Message ID: @.***>

zhangshixuan1987 commented 1 month ago

@lee1043 @tomvothecoder : Thank you both for the quick response to my questions and proposed the fix. I will update my library accordingly.