Skip to content

[ECmean] enable ecmean time selection#178

Merged
mcadau merged 4 commits intomainfrom
time-selection-ecmean
Mar 11, 2026
Merged

[ECmean] enable ecmean time selection#178
mcadau merged 4 commits intomainfrom
time-selection-ecmean

Conversation

@oloapinivad
Copy link
Collaborator

@oloapinivad oloapinivad commented Mar 6, 2026

PR description:

This addresses #168 by correctly propagating the selection at the retrieve level.


  • Tests are included if a new feature is included.
  • Changelog is updated.

@oloapinivad oloapinivad added the run tests Set this up to let test run label Mar 6, 2026
@oloapinivad oloapinivad changed the title enable ecmean time selection [ECmean] enable ecmean time selection Mar 6, 2026
@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.42%. Comparing base (63c7fc4) to head (64c7a0d).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
- Coverage   49.44%   49.42%   -0.02%     
==========================================
  Files          86       86              
  Lines        7326     7328       +2     
  Branches     1268     1267       -1     
==========================================
  Hits         3622     3622              
- Misses       3251     3253       +2     
  Partials      453      453              
Flag Coverage Δ
unittests 49.42% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Collaborator

@mnurisso mnurisso left a comment

Choose a reason for hiding this comment

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

changelog is missing but PR seems ready

@oloapinivad oloapinivad added ready to merge This PR is ready to merge in the opinion of the author and removed run tests Set this up to let test run labels Mar 9, 2026
@oloapinivad
Copy link
Collaborator Author

@mcadau @jhardenberg I realized it should be possible to revert the call in the reader_data call, so that now the regrid is done after the variable selection, and solve also: #159. Only issue is that there is no test for this, I am wondering if Marco can check if this working properly...

@mcadau
Copy link
Collaborator

mcadau commented Mar 9, 2026

Made some tests, with both 2 and 6 months of data (so still not enough to allow the diagnostic run so far).
In both cases, this is from the aqua analysis log:

Traceback (most recent call last):
  File "/work/bb1153/b383197/AQUA-diagnostics/aqua/diagnostics/ecmean/cli_ecmean.py", line 339, in <module>
    year1, year2 = time_check(data, year1, year2, logger=logger)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/work/bb1153/b383197/AQUA-diagnostics/aqua/diagnostics/ecmean/cli_ecmean.py", line 158, in time_check
    raise NotEnoughDataError("Not enough data, exiting...")
aqua.core.exceptions.NotEnoughDataError: Not enough data, exiting...

In addition, I confirm that in the current main the diagnostic wrongly runs errorless.

I also tested 12 months of data with this branch and no #159 errors occurred. So yes, you're right, and this change solves both issues.

If you agree we can merge.
Shall I proceed with operational porting of these changes?

@oloapinivad
Copy link
Collaborator Author

That's great! Thanks for testing... perhaps you can port this to the v0.19-operational (in AQUA?)? So that we can test it extensively in the next days and if everything is as we like it we can merge both!

@mcadau
Copy link
Collaborator

mcadau commented Mar 9, 2026

Yes no problem!

@mcadau
Copy link
Collaborator

mcadau commented Mar 11, 2026

Since #2761 worked fine and tests were good on the operational as well, we can merge it also for main

@mcadau mcadau merged commit d44b7d9 into main Mar 11, 2026
6 checks passed
@mcadau mcadau deleted the time-selection-ecmean branch March 11, 2026 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge This PR is ready to merge in the opinion of the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ECmean] startdate and enddate management [ECmean]: 3d ocean variable regridding errors

3 participants