search

CWSL / cwsl-mas

VisTrails plugin for Climate Model Analysis
Apache License 2.0
6 stars 32 forks source link

Field aggregation module #27

Closed DamienIrving closed 9 years ago

DamienIrving commented 9 years ago

I've been working on a whole bunch of new modules at my cwsl-sandbox repo and before I try and commit them all to the cwsl repo at once, I thought I'd try and implement just one and see if the wrapper I've developed is a suitable template to copy for all my other modules.

This particular module wraps cdo_field_agg.sh and when I try to run this workflow I get the following error message:

Traceback (most recent call last):
  File "/opt/cloudapps/vistrails/2.1.2/vistrails/core/modules/vistrails_module.py", line 400, in update
    self.compute()
  File "/home/599/dbi599/.vistrails/userpackages/cwsl/vt_modules/vt_field_agg.py", line 83, in compute
    this_process.execute(simulate=configuration.simulate_execution)
  File "/home/599/dbi599/.vistrails/userpackages/cwsl/core/process_unit.py", line 299, in execute
    final_command_list = self.apply_positional_args(keyword_command_list, this_dict)
  File "/home/599/dbi599/.vistrails/userpackages/cwsl/core/process_unit.py", line 339, in apply_positional_args
    this_att_value = constraint_dict[arg_name]
KeyError: 'method'

I'm wondering (a) why this error is happening (do my positional arguments need to have the same name as the constraints??) and (b) does my wrapper (vt_field_agg.py) look good as a template to copy for others?

captainceramic commented 9 years ago

Hi Damien - I'm not sure I can see exactly the same code that is running, but I think the reason you are having that problem is the API of positional arguments.

The way that it is written is that self.positional_args = [('agg_method', 0) ] means "For every command line you make, take the value of the 'agg_method' constraint and put it in as a positional argument directly after the command". I think what you want is to do self.positional_args = [(agg_method, 0, 'raw')] which instead takes the value of whatever string is held in agg_method and sets it as a positional argument.

For b), I think that looks good. There are two small things - the class variable keyword_args gets overwritten in the compute method, and the try/except at line 82 uses the old-style except KeyError, e instead of except KeyError as e - I would switch that and the e.output to match the one from the Nino vt module - see here

DamienIrving commented 9 years ago

Thanks @captainceramic - very useful feedback. I'll merge this one and use it as a template for the rest of the modules that I'm going to contribute.