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

bug fixes in process_en4() #609

Merged
merged 15 commits into from
Feb 27, 2023
Merged

bug fixes in process_en4() #609

merged 15 commits into from
Feb 27, 2023

Conversation

jpolton
Copy link
Collaborator

@jpolton jpolton commented Jan 23, 2023

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (./build.sh) was run locally and no errors reported. NB not sure about this requirement: GitActions test this
  • Lint (pylint .) has passed locally and any fixes were made for failures. NB not sure about this requirement: GitActions test this with black

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • [] Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue URL: #608

What is the new behavior?

  • nan masking of bad salinity point was misapplied using temperature mask locations:

      ds["practical_salinity"] = xr.where(~reject_tem_lev, ds["practical_salinity"], np.nan)
    

should read

    ds["practical_salinity"] = xr.where(~reject_sal_lev, ds["practical_salinity"], np.nan)

Also, though data points with bad values were masked with NaN, and profiles with both bad salinity and temperature were removed, profiles with bad temperature or salinity were not masked.

On inspection, other minor issues:

  • Method name change
    calculate_all_en4_qc_flags() only represents calculate_en4_qc_flags_levels(), and not profiles.
    Hence a name change/refactor

  • Duplicated methods in profile.py
    calculate_all_en4_qc_flags() appears twice
    process_en4() appears twice

  • In calculate_en4_qc_flags_levels():
    Different versions of EN dataset have a slightly different justitifcation of why some points are removed. This is inconsequential to the implementation of which points/profiles get masked but the code is updated to reflect this knowledge - perhaps making it more readable.

  • double mapping:
    Within profile.py (insitu) temperature was being mapped to both temperature and potential temperature. The variable should be preserved in this code and mappings happen exclusively in the json files

  • Add profile.mask_stats()

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing (pip install . && pytest unit_testing/unit_test.py -s)
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

Does this introduce a breaking change?

  • Yes
  • No

Other information

@jpolton jpolton linked an issue Jan 23, 2023 that may be closed by this pull request
@jpolton jpolton marked this pull request as ready for review February 27, 2023 11:33
@jpolton jpolton merged commit 19bf8c1 into develop Feb 27, 2023
@jpolton jpolton deleted the feature/606_profile_bug branch March 29, 2023 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Profile.process_en4() bug
1 participant