JCSDA-internal / eva

Evaluation and Verification of the Analysis
Apache License 2.0
5 stars 12 forks source link

Fix Level issue #195

Closed EdwardSafford-NOAA closed 3 months ago

EdwardSafford-NOAA commented 4 months ago

Stupidly simple fix, but took forever to find owing to a seriously unhelpful error message.

This fix enables conventional ps type plots to work correctly. Unlike other conventional time-series data, ps has only one level (surface, all others have 13). With the check on levels > 1 resulted in trying to assign coordinates of None for the dataset's Level dimension. Adding to the confusion the same error resulted from two different operations (assign_coordinates() and then a merge() of the date/time values). The actual error message was ValueError: dimension 'Level' already exists as a scalar variable, which really didn't describe the situation at all.

Closes #194 .

CoryMartin-NOAA commented 4 months ago

@EdwardSafford-NOAA any chance you can fix the unrelated issue that is causing the CI to fail np.Inf should be np.inf? I may be able to force a merge otherwise (I'm not sure)

EdwardSafford-NOAA commented 4 months ago

Having a look now.

kevindougherty-noaa commented 4 months ago

@CoryMartin-NOAA @EdwardSafford-NOAA This is the exact same issue I just debugged this morning. It is due to the fact that EMCPy's required matplotlib build is outdated. I updated in my PR to the newest version and it solved the CI issues. This may be related to what is happening on WCOSS2 as well and mentioned it to Cory this morning.

EdwardSafford-NOAA commented 4 months ago

Yes I spotted that too. I updated matplotlib to version 3.9.0 (latest afaik) in requirements.txt. That does not seem to have fixed the CI test though, so maybe there's more I need to do?

EdwardSafford-NOAA commented 4 months ago

Oh, I see additional places. Lemme fix that and retry.

srherbener commented 4 months ago

Not sure why I'm getting these, but a bunch of emails have come to me with the following message:

Branch name bug/level-194 Does not satisfy the Git-Flow naming conventions.
Please rename your local branch, push to GitHub, and delete branch bug/level-194.
Valid branch names begin with feature/ bugfix/ release/ or hotfix/

Repo: eva
Branch: bug/level-194
User: EdwardSafford-NOAA
User email: [62339196+EdwardSafford-NOAA@users.noreply.github.com](mailto:62339196%2BEdwardSafford-NOAA@users.noreply.github.com)

Apparently, we've got something watching the repos and trying to enforce git flow naming conventions. I think the watchdog wants the branch to be named bugfix/level-194 instead of bug/level-194. Don't worry about this PR, ie no need to change the branch name. I'm just sending this for future reference.

Thanks!

CoryMartin-NOAA commented 4 months ago

Thanks Steve, yeah I'm getting the same automated emails. @EdwardSafford-NOAA just an FYI for future reference

EdwardSafford-NOAA commented 4 months ago

@CoryMartin-NOAA I think I messed up something in my attempts to fix the CI issue. It looks like my change to mon_data_space.py is no longer in this PR. That's confusing because the change is in my bug[fix]/level-192 branch on wcoss2, and the branch reports as up to date with my origin/bug[fix]/level-192 branch. Not quite sure what I'm looking at or how/why my 1 char fix isn't showing up now.

CoryMartin-NOAA commented 4 months ago

@EdwardSafford-NOAA I think I may have already merged that into develop, perhaps this branch just needs made up to date with develop?

EdwardSafford-NOAA commented 4 months ago

@CoryMartin-NOAA Ok, I think I get it. You did merge my 1 char change and that is in develop now. My develop is current, and my bug[fix]/level-192 has develop merged into it. So everything is copacetic except for the ongoing CI issue.

EdwardSafford-NOAA commented 3 months ago

With the level fix now in develop I'm going to close this PR. The CI error has been resolved by updating the emcpy hash and the matlabplot version.