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

Fix Level issue #195

Closed
wants to merge 7 commits into from
Closed

Fix Level issue #195

wants to merge 7 commits into from

Conversation

EdwardSafford-NOAA
Copy link
Contributor

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 .

@EdwardSafford-NOAA
Ref #194
8e6ecd3 
Fixed.
@CoryMartin-NOAA
Copy link
Collaborator

@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
Copy link
Contributor Author

Having a look now.

@kevindougherty-noaa
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@EdwardSafford-NOAA
Ref #194
37058c2 
Maybe fix CI issues now.
@EdwardSafford-NOAA
Ref #194
de46c3a 
Retry CI fix.
@srherbener
Copy link

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: [[email protected]](mailto:62339196%[email protected])

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
Copy link
Collaborator

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

@EdwardSafford-NOAA
Copy link
Contributor Author

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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.

@EdwardSafford-NOAA EdwardSafford-NOAA deleted the bug/level-194 branch July 12, 2024 13:26
@EdwardSafford-NOAA EdwardSafford-NOAA mentioned this pull request Jul 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CoryMartin-NOAA CoryMartin-NOAA CoryMartin-NOAA approved these changes

@ADCollard ADCollard Awaiting requested review from ADCollard

@kevindougherty-noaa kevindougherty-noaa Awaiting requested review from kevindougherty-noaa

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

Successfully merging this pull request may close these issues.

[Bug] mon_data_space.py won't load ps data files
4 participants
@EdwardSafford-NOAA @CoryMartin-NOAA @kevindougherty-noaa @srherbener