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

allow empyt value for optional numeric inputs #19

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bernt-matthias
Copy link
Contributor

@bernt-matthias bernt-matthias commented Jul 6, 2019

The CTDConverter fails for value="" for numeric non-mandatory inputs:

  File "CTDConverter/convert.py", line 215, in main
    converter.get_preferred_file_extension())
  File "/home/berntm/projects/tools-galaxyp/tools/openms/gen-test/CTDConverter/common/utils.py", line 118, in parse_input_ctds
    parsed_ctds.append(ParsedCTD(CTDModel(from_file=input_ctd), input_ctd, output_file))
  File "/home/berntm/projects/tools-galaxyp/tools/openms/gen-test/CTDopts/CTDopts/CTDopts.py", line 643, in __init__
    self._load_from_file(from_file)
  File "/home/berntm/projects/tools-galaxyp/tools/openms/gen-test/CTDopts/CTDopts/CTDopts.py", line 688, in _load_from_file
    self.parameters = self._build_param_model(params_container_node, base=None)
  File "/home/berntm/projects/tools-galaxyp/tools/openms/gen-test/CTDopts/CTDopts/CTDopts.py", line 708, in _build_param_model
    self._build_param_model(child, current_group)
  File "/home/berntm/projects/tools-galaxyp/tools/openms/gen-test/CTDopts/CTDopts/CTDopts.py", line 708, in _build_param_model
    self._build_param_model(child, current_group)
  File "/home/berntm/projects/tools-galaxyp/tools/openms/gen-test/CTDopts/CTDopts/CTDopts.py", line 713, in _build_param_model
    base.add(**setup)  # register parameter in model
  File "/home/berntm/projects/tools-galaxyp/tools/openms/gen-test/CTDopts/CTDopts/CTDopts.py", line 564, in add
    self.parameters[name] = Parameter(name, self, **kwargs)
  File "/home/berntm/projects/tools-galaxyp/tools/openms/gen-test/CTDopts/CTDopts/CTDopts.py", line 376, in __init__
    self._validate_numerical_defaults(default)
  File "/home/berntm/projects/tools-galaxyp/tools/openms/gen-test/CTDopts/CTDopts/CTDopts.py", line 456, in _validate_numerical_defaults
    "default": ', '.join(map(str, errors_so_far))})
ModelParsingError: An error occurred while parsing the CTD file: Invalid default value(s) provided for parameter param_wodefault_mandatory_unrestricted of type <type 'float'>: ''
ERROR: There seems to be a problem with one of your input CTDs.

Not sure if there is a better way to do it (e.g. alternatively "" could be removed from the list)

@bernt-matthias bernt-matthias changed the title allow empyt value for numeric inputs allow empyt value for optional numeric inputs Jul 15, 2019
@bernt-matthias bernt-matthias force-pushed the topic/empty_numeric_defaults branch from d839fbd to 30f525a Compare July 15, 2019 12:29
@bernt-matthias
Copy link
Contributor Author

@b-schubert @andras86 @bgruening : any chance to get this merged?

@jpfeuffer
Copy link
Collaborator

In which cases is this needed?
Shouldn't the parameter then be skipped completely?

@bernt-matthias
Copy link
Contributor Author

Happens for instance for this parameter:

      <ITEM name="wodefault_mandatory_unrestricted"     value=""    type="double" description="float" required="true"  advanced="false" />

I assumed in my test cases that there might be mandatory arguments for which no default is available.

@jpfeuffer
Copy link
Collaborator

Hmm I dont know if this should happen. If I designed a tool, I would avoid such parameters.
With string parameters this wont be distinguishable from an empty string for example. In case of string parameters with restrictions this is even more complicated (you would need to add the empty string as a possible selection).
Do you have any real-world examples of such tools, where a reasonable default cannot be given?

@bernt-matthias
Copy link
Contributor Author

I had a tool developer in mind who want's to implement a parameter for which no useful default exists, but depends in a non-computable way on the input. Such a parameter could be for instance a taxon name for choosing a reference data base.

Or a case where the developer wants to "force" the user to think about the parameter.

@jpfeuffer
Copy link
Collaborator

I mean in general I do not care much. But you need to be careful with dependent programs/code that do a validation on provided CTDs (e.g. GenericKnimeNodes). They will probably start to complain before the user even does anything.
And there is still the problem of how to differentiate between an empty string for string parameters.

@bernt-matthias
Copy link
Contributor Author

Seems to boil down to the question if the empty string (ie the default given by value="") is acceptable if its a required parameter.

I assumed that tools won't accept that the user leaves this empty and only run when a value has been entered.

and shorten/empty if necessary
@timosachsenberg
Copy link
Collaborator

I think we had quite some trouble in the past with optional vs. empty parameters. I would only add this if really required

@bernt-matthias
Copy link
Contributor Author

Currently I can live with it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants