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

Refactored native model fixes by adding common base class NativeDatasetFix #1694

Merged
merged 7 commits into from
Aug 16, 2022

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Aug 9, 2022

Description

This PR simplifies native model fixes by adding a base class NativeDatasetFix with common operations necessary for almost all datasets. This will dramatically simplify #1678.

Closes #1690

Link to documentation: https://esmvaltool--1694.org.readthedocs.build/projects/ESMValCore/en/1694/develop/fixing_data.html#fix-native-data


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #1694 (803f4e8) into main (76c9ffa) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1694      +/-   ##
==========================================
+ Coverage   91.49%   91.51%   +0.01%     
==========================================
  Files         204      205       +1     
  Lines       11174    11191      +17     
==========================================
+ Hits        10224    10241      +17     
  Misses        950      950              
Impacted Files Coverage Δ
esmvalcore/cmor/_fixes/emac/_base_fixes.py 100.00% <100.00%> (ø)
esmvalcore/cmor/_fixes/emac/emac.py 100.00% <100.00%> (ø)
esmvalcore/cmor/_fixes/icon/_base_fixes.py 100.00% <100.00%> (ø)
esmvalcore/cmor/_fixes/icon/icon.py 100.00% <100.00%> (ø)
esmvalcore/cmor/_fixes/native_datasets.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@schlunma schlunma self-assigned this Aug 10, 2022
@schlunma schlunma added the fix for dataset Related to dataset-specific fix files label Aug 10, 2022
@schlunma schlunma added this to the v2.7.0 milestone Aug 10, 2022
@schlunma schlunma marked this pull request as ready for review August 10, 2022 07:40
@schlunma schlunma requested a review from remi-kazeroni August 10, 2022 07:48
@schlunma
Copy link
Contributor Author

I have no idea why that Codacy error (Django was not configured. For more information run pylint --load-plugins=pylint_django --help-msg=django-not-configured) appears here. I think we can simply ignore it...

Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution @schlunma! The code looks good to me. This should help lowering the bar to add support for native model output in the future. This is also a good step towards reducing code duplication w.r.t. model fixes. The CMORization of models related to this PR gives identical results to that of the main branch and other models are not affected (different classes) so I think this is ready to be merged 👍

@remi-kazeroni
Copy link
Contributor

I have no idea why that Codacy error (Django was not configured. For more information run pylint --load-plugins=pylint_django --help-msg=django-not-configured) appears here. I think we can simply ignore it...

I have seen that in some other unrelated PRs as well, so I would agree...

@schlunma
Copy link
Contributor Author

Awesome, thanks for the review @remi-kazeroni ! 🚀

@schlunma schlunma merged commit f694d14 into main Aug 16, 2022
@schlunma schlunma deleted the refactor_native_dataset_fixes branch August 16, 2022 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix for dataset Related to dataset-specific fix files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor native model fixes
2 participants