Development fx restructured #21

valeriupredoi · 2019-06-10T15:34:35Z

Related issue #109
porting ESMValGroup/ESMValTool#1065 to ESMValCore/version2_development

…RUCTURED

bouweandela · 2019-06-14T08:45:58Z

Can you please merge the development branch into this branch, so we can see what's new?

valeriupredoi · 2019-06-14T14:01:06Z

Can you please merge the development branch into this branch, so we can see what's new?

le done 🍺

esmvalcore/_data_finder.py

bouweandela · 2019-06-14T14:35:06Z

esmvalcore/_data_finder.py

    files = find_files(input_dirs, filenames_glob)

    return files


 def get_input_filelist(variable, rootpath, drs):
    """Return the full path to input files."""
+    # change ensemble to fixed r0i0p0 for fx variables
+    if variable['project'] == 'CMIP5'and variable['frequency'] == 'fx':
+        variable['ensemble'] = 'r0i0p0'


You're already setting the ensemble in _recipe.py, so I think there is no need to do it again here

ermm, I don't think that gets picked up for all the cases, lemme test

Have you had a chance to do this yet?

not yet 😢

bouweandela · 2019-06-14T14:41:50Z

I think we should also get rid of the funky syntax for specifying fx variables. So instead of this (example from recipe_ocean_Landschuetzer2016.yml):

diagnostics:
  diag_timeseries_surface_average_vs_OBS:
    variables:
      dpco2:
        mip: Omon
        fx_files: [areacello, ]
      spco2:
        mip: Omon
        fx_files: [areacello, ]
      fgco2:
        mip: Omon
        fx_files: [areacello, ]

we should have

diagnostics:
  diag_timeseries_surface_average_vs_OBS:
    variables:
      dpco2:
        mip: Omon
      spco2:
        mip: Omon
      fgco2:
        mip: Omon
      areacello:
        mip: Omon

If we do it like this, it has the advantage that fixes and cmor checks are applied automatically to fx variables in the same way as they are applied to normal variables. The only remaining issue is then how to apply fixes to fx variables that are used as input to preprocessor functions, but that is something we can solve later.

Does this make sense?

valeriupredoi · 2019-06-14T15:07:55Z

yes, I like that and I initially wanted to do it this way but the problem is that the preprocessor masking with fx is way more frequent in a lot of recipes than just using the fx files/variables in the diagnostics only so we need that functionality more stringently - I think we should probably leave it as is now for now, since this way (this PR) mirrors the previous functionality (with the added funky syntax yes) and set this up for a change for v2.1. What say you @mattiarighi @bouweandela @ledm

valeriupredoi · 2019-06-14T15:09:54Z

#22 is actually doing more so might wanna look at this instead

bouweandela · 2019-06-17T07:37:09Z

Hi V., I though that the simplification of the variable definition as described in in #21 (comment) was the whole point of this pull request. If not, what problem is this actually solving? Is there an associated issue?

valeriupredoi · 2019-06-18T10:29:14Z

@bouweandela (sorry man, I was travelling yesterday, am fully back in work now) - this PR does a few things:

simplifies the code base by removing the need for dedicated fx file finding functions in _data_finder.py and dedicated structures in config-developer.yml
implements the usage for CMIP6 fx files that now have mips that are not just fx
allows for using the fx variable as a stand-alone standard variable
carries over the functionality for CMIP5 fx files finding and usage

Note that to have the usage of fx variables and files in the preprocessor via mask_landsea etc without explicitly declaring the fx dictionary fx_files: {'short_name': 'bla', 'mip': 'blabla'} is not trivial to implement since _recipe.py treats variables on an independent base. #22 is stepping towards that

Alas, there is no issue for this PR but there is one for both this PR and #22 in #46

valeriupredoi · 2019-06-20T12:40:30Z

#109

bouweandela · 2019-06-26T13:44:48Z

esmvalcore/_data_finder.py

    files = find_files(input_dirs, filenames_glob)

    return files


 def get_input_filelist(variable, rootpath, drs):
    """Return the full path to input files."""
+    # change ensemble to fixed r0i0p0 for fx variables
+    if variable['project'] == 'CMIP5'and variable['frequency'] == 'fx':


Suggested change

if variable['project'] == 'CMIP5'and variable['frequency'] == 'fx':

if variable['project'] == 'CMIP5' and variable['frequency'] == 'fx':

bouweandela · 2019-06-26T13:45:52Z

esmvalcore/_recipe.py

@@ -361,6 +362,45 @@ def _get_default_settings(variable, config_user, derive=False):
    return settings


+def get_input_fx_filelist(variable, rootpath, drs):


I think this function should be removed altogether, it looks like it migrated here from _data_finder.py

reply below in the main body of the issue

bouweandela · 2019-06-26T13:51:11Z

yes, I like that and I initially wanted to do it this way but the problem is that the preprocessor masking with fx is way more frequent in a lot of recipes than just using the fx files/variables in the diagnostics only so we need that functionality more stringently - I think we should probably leave it as is now for now, since this way (this PR) mirrors the previous functionality (with the added funky syntax yes) and set this up for a change for v2.1. What say you

I would prefer to have this done in this pull request. Because now it looks like a cleanup operation that is only half done, you remove the get_input_fx_filelist function from _data_finder.py, only to re-introduce it in _recipe.py.

valeriupredoi · 2019-06-26T14:11:39Z

the problem is that we are still keeping the fx_files dictionary attached to any variable that needs it or created by the request of fx masking in the preprocessor - you can not remove this function if the fx_files is still needed. This function merely creates that dictionary and populates it with key (fx variable) and values (fx files). What you are proposing is, as I said before, nice and straightforward but a nightmare to implement in real life. Have a look at #109 and tell me if you have a good and easy way to couple VAR with FXVAR 🍺

bouweandela · 2019-06-26T14:23:32Z

Which preprocessor function requires a dictionary with fx files? The ones in esmvalcore/preprocessor/_mask.py only require a list of files, which can simply be returned by get_input_filelist

bouweandela · 2019-06-26T14:24:41Z

Have a look at #109 and tell me if you have a good and easy way to couple VAR with FXVAR

That's another discussion. I've written an answer in the issue.

valeriupredoi · 2019-06-26T14:29:00Z

Which preprocessor function requires a dictionary with fx files? The ones in esmvalcore/preprocessor/_mask.py only require a list of files, which can simply be returned by get_input_filelist

mask_landsea and mask_lanseaice need specific fx variables (sftlf, sftof, sftgif) and yes, they can be lists of files but they need to be attached to the variable that needs masking which brings us back to the issue in #109

valeriupredoi · 2019-06-26T14:33:11Z

Have a look at #109 and tell me if you have a good and easy way to couple VAR with FXVAR

That's another discussion. I've written an answer in the issue.

yes, I saw it and makes sense but that's not gonna be easy-peasy. Would you rather soend time on that or allow a kludge like this one here. For a nice functional way we should get the independent fx variables as regular variables implementation but note that now, as things stand in the trunk, CMIP6 fx variables dont work at all so we need something in the meantime. I am buried under documentation at the moment and I can restart this only next week. Also, have you seen #22 - that fixes all the problems bar removing the fx_files dict, which can probably be done quickly

bouweandela · 2019-06-26T15:01:39Z

removing the fx_files dict, which can probably be done quickly

If you could do that in this pull request would be great.

Also, have you seen #22

I believe that pull request includes the changes in this pull request plus extra stuff, right? If so, I think this pull request should be finished before we start on the other one.

valeriupredoi · 2019-06-26T15:34:43Z

removing the fx_files dict, which can probably be done quickly

If you could do that in this pull request would be great.

🐫 🍺

ie I'll try next week, I am knocking down the dox now, see #113 😁

valeriupredoi · 2019-08-08T14:33:00Z

superseded for now by #170

valeriupredoi added 17 commits May 12, 2019 13:16

removed all traces of specific fx file retrieval

5068594

do time checks only if not fx variable

909e34f

removed references to fx files and dirs

1bbf364

removed fx files function

3e63424

restructuring to allow for fx vars treated like any other var

33b9066

cleaning up

5614f7c

cleaning up

475cd8f

fixed test for new fx file retrieval

0ee4a72

added checks for no time gating

6989951

fixed the test to reflect the new standards

988d09c

fixed the test to reflect the new standards

775715c

fixed the data structure standard to reflect new standards

32b6376

Merge branch 'version2_development' into version2_development_fx_REST…

d50465c

…RUCTURED

fixed little bug

2ff537d

Merge branch 'version2_development' into version2_development_fx_REST…

62922ed

…RUCTURED

removed stray import from conflict

b36f312

Merge branch 'development' into development_fx_RESTRUCTURED

1522c89

valeriupredoi added preprocessor Related to the preprocessor paper labels Jun 10, 2019

valeriupredoi requested review from bouweandela and mattiarighi June 10, 2019 15:34

valeriupredoi added 2 commits June 10, 2019 17:04

fixed stray import mistakes introduced by merge

cf05187

removed unused import

e799ec8

Merge branch 'development' into development_fx_RESTRUCTURED

1a4a766

removed unused import

7ab878f

bouweandela reviewed Jun 14, 2019

View reviewed changes

esmvalcore/_data_finder.py Outdated Show resolved Hide resolved

bouweandela reviewed Jun 14, 2019

View reviewed changes

removed useless variable

0fc4392

bouweandela mentioned this pull request Jun 19, 2019

Zonal and meridional statistics preprocessor #24

Closed

9 tasks

This was referenced Jun 20, 2019

Keep the meeting banter going. #10

Closed

FX variables saga #109

Closed

bouweandela reviewed Jun 26, 2019

View reviewed changes

Merge branch 'development' into development_fx_RESTRUCTURED

6f51e4b

valeriupredoi mentioned this pull request Jul 17, 2019

Working FX handling #170

Merged

valeriupredoi closed this Aug 8, 2019

valeriupredoi deleted the development_fx_RESTRUCTURED branch November 7, 2019 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Development fx restructured #21

Development fx restructured #21

valeriupredoi commented Jun 10, 2019 •

edited

Loading

bouweandela commented Jun 14, 2019

valeriupredoi commented Jun 14, 2019

bouweandela Jun 14, 2019

valeriupredoi Jun 14, 2019

bouweandela Jun 26, 2019

valeriupredoi Jun 26, 2019

bouweandela commented Jun 14, 2019 •

edited

Loading

valeriupredoi commented Jun 14, 2019

valeriupredoi commented Jun 14, 2019

bouweandela commented Jun 17, 2019

valeriupredoi commented Jun 18, 2019

valeriupredoi commented Jun 20, 2019

bouweandela Jun 26, 2019

valeriupredoi Jun 26, 2019

bouweandela Jun 26, 2019

valeriupredoi Jun 26, 2019

bouweandela commented Jun 26, 2019

valeriupredoi commented Jun 26, 2019

bouweandela commented Jun 26, 2019 •

edited

Loading

bouweandela commented Jun 26, 2019

valeriupredoi commented Jun 26, 2019

valeriupredoi commented Jun 26, 2019

bouweandela commented Jun 26, 2019

valeriupredoi commented Jun 26, 2019 •

edited

Loading

valeriupredoi commented Aug 8, 2019

	if variable['project'] == 'CMIP5'and variable['frequency'] == 'fx':
	if variable['project'] == 'CMIP5' and variable['frequency'] == 'fx':

		@@ -361,6 +362,45 @@ def _get_default_settings(variable, config_user, derive=False):
		return settings


		def get_input_fx_filelist(variable, rootpath, drs):

Development fx restructured #21

Development fx restructured #21

Conversation

valeriupredoi commented Jun 10, 2019 • edited Loading

bouweandela commented Jun 14, 2019

valeriupredoi commented Jun 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bouweandela commented Jun 14, 2019 • edited Loading

valeriupredoi commented Jun 14, 2019

valeriupredoi commented Jun 14, 2019

bouweandela commented Jun 17, 2019

valeriupredoi commented Jun 18, 2019

valeriupredoi commented Jun 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bouweandela commented Jun 26, 2019

valeriupredoi commented Jun 26, 2019

bouweandela commented Jun 26, 2019 • edited Loading

bouweandela commented Jun 26, 2019

valeriupredoi commented Jun 26, 2019

valeriupredoi commented Jun 26, 2019

bouweandela commented Jun 26, 2019

valeriupredoi commented Jun 26, 2019 • edited Loading

valeriupredoi commented Aug 8, 2019

valeriupredoi commented Jun 10, 2019 •

edited

Loading

bouweandela commented Jun 14, 2019 •

edited

Loading

bouweandela commented Jun 26, 2019 •

edited

Loading

valeriupredoi commented Jun 26, 2019 •

edited

Loading