Add YearProperty descriptor to automatically pad years <1000 #973
Conversation
|
@tomvothecoder, inspired by your comment #972 (comment). I attempted this implementation, though there is still somehow the online CI/CD test errors. Maybe we should go for this approach, that is more automatic? I'm trying to fix my local environment to match the CI env, to have consistent results, any suggestion is appreciated! |
Thanks for building on my PR! Yes, an automated way of detecting when to pad years is a great idea. I'll review your PR when I get the chance. |
Update references to use `pad_year` where time slicing is required using `.sel` to ensure year is padded
- Add YearProperty descriptor class for automated year padding - Add property descriptors for all year attributes (start_yr, end_yr, test_start_yr, test_end_yr, ref_start_yr, ref_end_yr) - Add unit test for automatic year padding functionality - Uses the pad_year utility function for consistent validation and padding This enhancement makes year padding automatic and transparent to users, ensuring all year values are properly formatted as 4-digit strings for ISO-8601 compliance and xarray time slicing.
tomvothecoder
force-pushed
the
bug/971-iso-8601-dates-enhanced
branch
from
0dea86e to
36fd6a3
Compare
- Logic in `_add_parent_attrs_to_children()` is updated to ignore YearProperty attributes that are not set on the parent object (e.g., CoreParameter). This prevents an AttributeError from being raised - Update `_add_attrs_with_default_values()` to only add default attribute values to the child parameter if the attributes are set on the parent object - Update `YearProperty` __get__ to handle when attribute is not set and update __set__ to handle None values
There was a problem hiding this comment.
This is a clean and thoughtful implementation. Nice work! Unfortunately, it runs into some issues with how e3sm_diags handles parameter objects, specifically with the YearProperty attributes.
The main problem is that these YearProperty attributes show up in dir(parent_param) (list of all attributes) even when they haven’t actually been set. That causes trouble in run.py when we try to pass attributes between parent and child parameter objects. It breaks logic that depends on checking whether an attribute exists or not, because technically it looks like it does, but it really doesn’t.
I added a few workarounds to get the unit tests to pass, but some integration tests are still failing with AttributeError: __delete__ for the YearProperty attributes. I have not found a clear solution for this issue. The current logic for propagating attributes is already pretty complex, and these edge cases make it even harder to get right without adding more complexity to the existing logic.
One simpler alternative could be to just call pad_year on these attributes in CoreParameter.__init__() if they’re explicitly set. It is not as clean as automating this process with YearProperty, but it might be more compatible with the current run.py code. I will explore this route in my PR #972.
|
Use solution is #973 instead. |
Description
Branch from #972
Checklist
If applicable: