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.

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

Fix redundant _FillValue attribute for MPAS fields #259

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

xylar
Copy link
Contributor

@xylar xylar commented May 16, 2024

Previously, the write_netcdf() function was trying to add a _FillValue attribute to variables even if they already had one. With this change, we skip variables that already have a fill value defined

Description

  • Closes #<ISSUE_NUMBER_HERE>

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

If applicable:

  • New and existing unit tests pass with my changes (locally and CI/CD build)
  • I have added tests that prove my fix is effective or that my feature works
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

Previously, the `write_netcdf()` function was trying to add
a `_FillValue` attribute to variables even if they already had one.
With this change, we skip variables that already have a fill
value defined
@xylar xylar added the bug Something isn't working label May 16, 2024
@xylar xylar requested review from chengzhuzhang and TonyB9000 May 16, 2024 01:49
@xylar
Copy link
Contributor Author

xylar commented May 16, 2024

@chengzhuzhang and @TonyB9000, I'm not really in a good position to test this. Can one of you give it a try on the data that was failing for @TonyB9000 before? I'm happy to iterate if needed.

@xylar
Copy link
Contributor Author

xylar commented May 16, 2024

I'm not sure if this addresses #255 completely or if there are multiple bugs there. I left it out of the description above for now.

@xylar
Copy link
Contributor Author

xylar commented May 16, 2024

It looks like CI is broken and it's nothing to do with my PR?

@TonyB9000
Copy link
Contributor

@xylar I approve. The change looks perfectly harmless to me. I will run tests to see if this served to address the fault.

@chengzhuzhang
Copy link
Collaborator

@TonyB9000 have you confirmed that Xylar's fix has resolved this issue?

@TonyB9000
Copy link
Contributor

@chengzhuzhang Yes - all of the v2_1 Omon (20+) that were failing were able to complete successfully, I should have followed up here - got distracted with other issues. Of course, why this would be an issue (adding a _fillValue to variables that already have one) is a mystery, but checking first and avoiding the "add" solved the problem.

@chengzhuzhang chengzhuzhang merged commit 062072f into E3SM-Project:master Jun 24, 2024
0 of 3 checks passed
@chengzhuzhang
Copy link
Collaborator

@chengzhuzhang Yes - all of the v2_1 Omon (20+) that were failing were able to complete successfully, I should have followed up here - got distracted with other issues. Of course, why this would be an issue (adding a _fillValue to variables that already have one) is a mystery, but checking first and avoiding the "add" solved the problem.

@TonyB9000 thank you for confirming. And thank you @xylar for the fix!

@TonyB9000
Copy link
Contributor

@chengzhuzhang @xylar This issue (#259) and issue (#255) appear related and fixed. However, #255 also overlapped with the "lock=True/lock=False" issue, which (I believe) was resolved by completely eliding the inclusion of "lock=" in the command where it had appeared. Should we close these issues?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants