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.

Sign up for GitHub

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

Jump to bottom

Add valid_time to TIME_NAMES and fix issue with pretty printing #206

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add valid_time to TIME_NAMES and fix issue with pretty printing #206

wants to merge 3 commits into from

Conversation

ph-kev
Copy link
Member

@ph-kev ph-kev commented Feb 28, 2025

closes #204, closes #205 - The issue with pretty printing is when the OutputVar has empty dictionaries in which the padding cannot be computed. The other potential issue is when the value of dim_attributes is something that is not a dictionary.

@ph-kev ph-kev marked this pull request as ready for review February 28, 2025 18:51
@ph-kev ph-kev requested a review from Sbozzolo February 28, 2025 18:51
@codecov Codecov
Copy link

codecov bot commented Feb 28, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 98.10%. Comparing base (7513972) to head (712fa64).

Files with missing lines Patch % Lines
src/Var.jl 80.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #206      +/-   ##
==========================================
- Coverage   98.25%   98.10%   -0.16%     
==========================================
  Files          12       12              
  Lines        1263     1266       +3     
==========================================
+ Hits         1241     1242       +1     
- Misses         22       24       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sbozzolo
Copy link
Member

I don't think there should be a case where dim_attributes is not a dict. It's type is OrderedDict{String, C} in the type definition

@ph-kev
Copy link
Member Author

ph-kev commented Feb 28, 2025

I don't think there should be a case where dim_attributes is not a dict. It's type is OrderedDict{String, C} in the type definition

I realized that I poorly wrote that sentence. I mean that C can be something that is not a dictionary.

@ph-kev ph-kev force-pushed the kp/bugs branch 2 times, most recently from b8d7cae to 656232c Compare March 1, 2025 00:30
@Sbozzolo
Copy link
Member

Sbozzolo commented Mar 3, 2025

I don't think there should be a case where dim_attributes is not a dict. It's type is OrderedDict{String, C} in the type definition

I realized that I poorly wrote that sentence. I mean that C can be something that is not a dictionary.

Are we encountering these cases in the wild? Which other Cs do we have to deal with?

@ph-kev
Copy link
Member Author

ph-kev commented Mar 3, 2025

I don't think there should be a case where dim_attributes is not a dict. It's type is OrderedDict{String, C} in the type definition

I realized that I poorly wrote that sentence. I mean that C can be something that is not a dictionary.

Are we encountering these cases in the wild? Which other Cs do we have to deal with?

I don't think this ever appear in the wild, but we don't restrict what C could be in the struct for an OutputVar. I agree that there shouldn't be a case where dim_attributes is not a Dict. Should I add the constraint for what C is in the OutputVar?

@Sbozzolo
Copy link
Member

Sbozzolo commented Mar 3, 2025

I don't think there should be a case where dim_attributes is not a dict. It's type is OrderedDict{String, C} in the type definition

I realized that I poorly wrote that sentence. I mean that C can be something that is not a dictionary.

Are we encountering these cases in the wild? Which other Cs do we have to deal with?

I don't think this ever appear in the wild, but we don't restrict what C could be in the struct for an OutputVar. I agree that there shouldn't be a case where dim_attributes is not a Dict. Should I add the constraint for what C is in the OutputVar?

Yes, I'd say that would be the safest thing to do.

ph-kev added 3 commits March 3, 2025 16:26
@ph-kev
Make dim_attribs map string to dicts
25d849e
@ph-kev
Add valid_time to TIME_NAMES
634ac24
@ph-kev
Fix bug with pretty printing OutputVar
712fa64 
If a `Outputvar` is empty, then there was an issue with computing the
amount of padding to add. This can be fixed by adding `init = 0` when
taking the maximum, so the amount of padding is 0 if there is nothing to
pad.  Additionally, if there are 0 elements in a dimension, it print "0
element" instead of just "0". Finally, when printing the dimensions that
the data is defined over, we do not add a new line on the final line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sbozzolo Sbozzolo Awaiting requested review from Sbozzolo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add compatibility for reading NetCDF files from ERA5 Pretty print when dictionaries are empty throw an error
2 participants
@ph-kev @Sbozzolo