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

Add optional default to Category node #33

Merged
merged 5 commits into from
Feb 17, 2021
Merged

Add optional default to Category node #33

merged 5 commits into from
Feb 17, 2021

Conversation

nsmith-
Copy link
Collaborator

@nsmith- nsmith- commented Feb 15, 2021

Closes #1
Also clean up handling of optional attributes in evaluator

Closes #1
Also clean up handling of optional attributes in evaluator
@nsmith-
Copy link
Collaborator Author

nsmith- commented Feb 15, 2021

@IzaakWN does this work for your case?
I remember we talked about about specifying the default in the Variable but I decided against that, based on the fact that all variable types are default constructible so if the user wants to put in a dummy value they can.
The keyword-only evaluate signature we talked about can still be implemented by just default-constructing the missing arguments. Perhaps still we can add a boolean required flag to the input Variables but at least it is orthogonal to putting defaults in the nodes.

@IzaakWN
Copy link
Contributor

IzaakWN commented Feb 17, 2021

Yes, the default field in Category looks good to me!

Regarding a default value in Variable, I don't think it would be strictly necessary, but we can think about it in the future if it would not be too difficult. Without a default input, users will be forced to chose think about what dummy value to pass, which can be either good or bad.

@nsmith-
Copy link
Collaborator Author

nsmith- commented Feb 17, 2021

I think we can make a nice solution for default Variable input by allowing a sentinel object in the variant for the evaluate signature, e.g.:

double Correction::evaluate(const std::vector<std::variant<int, double, std::string, Variable::default>>& values) const;

But I will save that for another PR.

I think also that, after adding the binning overflow in #37, it might make sense to allow the default value to be any Content node, that way it is clearly separated from the valid keys in the mapping. Something like default: Optional[Content]

@nsmith-
Copy link
Collaborator Author

nsmith- commented Feb 17, 2021

We could also have

default: Union[int, str, Content]

so that one could re-use a key or add a new node. The question is whether this is prone to abuse: after all, if there is a default, it need not have a key. But I could also see an argument for having a "nominal" key.

@nsmith-
Copy link
Collaborator Author

nsmith- commented Feb 17, 2021

I settled on default: Optional[Content] since:

  • A category with no default will always throw on invalid element
  • A category with a named default value can simply omit the named value from the regular items, as the missing name will resolve to the default
  • A category with a named value that has some significance needs to handle the name at a different level (e.g. for a systematic node (New node type for systematic variation #4), it might have a Category subnode and descend "nominal" and "shift" category items to return a shifted value)

@nsmith- nsmith- merged commit c3710d4 into master Feb 17, 2021
@nsmith- nsmith- deleted the feat-category branch February 17, 2021 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Default category
2 participants