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

Smaller changes to parameters #788

Merged
merged 25 commits into from
Oct 16, 2017

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Oct 10, 2017

This includes the changes from #774 and changes the set/get to imho be more logical
and resolves issues regarding using val_mapping and parers on parameters without a set/get method (What used to be manual paramters) (see test)

  • Add typing to init functions
  • Set private variables in constructors rather than using setters this fixes a number of warnings
  • Change raw_value to unconditionally store the variable in machine format, this means that the get parser, inverse scaling and inverse val_mapping can always be applied
  • change the order of finding step size and performing scaling, mapping etc. this means that steps are calculated in the unit of the user input which imho is more locical and enables calculation of raw_values for all steps
  • rename the wrapped get/set methods to get_raw and set_raw.

Copy link
Contributor

@WilliamHPNielsen WilliamHPNielsen left a comment

Choose a reason for hiding this comment

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

I think this looks good.

@nulinspiratie
Copy link
Contributor

Hey @jenshnielsen had a look, and the changes look good! I also stumbled on a couple of them.
Only two things I came up with were

  1. if we set the private variables in the constructor, won't this mean that the checks aren't performed? E.g. a parameter could have a negative step size if specified during initialization. What warnings did it give, and is there another way to deal with them?
  2. I like the idea of being able to access the unwrapped get/set method via get_raw/set_raw, but this now only holds if created via the get_cmd/set_cmd. If we subclass the BaseParameter with a custom get/set we cannot access it via get_raw/set_raw. Would it be an idea to also specify get_raw/set_raw in BaseParameter (self.get_raw = self.get)?

@jenshnielsen
Copy link
Collaborator Author

  1. It was a linter warning. I have reverted if for now. We can always look into this later
  2. The logic to actually wrap the parameter is only implemented in Parameter, not in BaseParamter so if you subclass BaseParamter you will have to explicitly wrap your get command. You are of course free to use the wrapper in your subclass anyway as in you can implement a get_raw and wrap it in the subclass or implement a get command and either wrap it or not as works for your parameter.

@nulinspiratie
Copy link
Contributor

nulinspiratie commented Oct 12, 2017 via email

@jenshnielsen
Copy link
Collaborator Author

Sorry missed that. Should be fixed now

@WilliamHPNielsen WilliamHPNielsen merged commit 6bfb22d into microsoft:master Oct 16, 2017
giulioungaretti pushed a commit that referenced this pull request Oct 16, 2017
Author: Jens Hedegaard Nielsen <jenshnielsen@gmail.com>

    Smaller changes to parameters (#788)
giulioungaretti pushed a commit that referenced this pull request Oct 16, 2017
Author: Jens Hedegaard Nielsen <jenshnielsen@gmail.com>

    Smaller changes to parameters (#788)
@jenshnielsen jenshnielsen deleted the fix_parameters branch October 16, 2017 08:29
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