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

set_parser for seq element setting added #832

Merged
merged 4 commits into from
Nov 3, 2017

Conversation

jenshnielsen
Copy link
Collaborator

@nataliejpg Cleanup of #828 are there any relevant changed that I missed

@jenshnielsen jenshnielsen mentioned this pull request Oct 31, 2017
@jenshnielsen
Copy link
Collaborator Author

@WilliamHPNielsen Any inside if this makes sense

@Dominik-Vogel
Copy link
Contributor

@nataliejpg do you remember why you added the set parser?

@nataliejpg
Copy link
Contributor

there was an error when I tried to set a jump to which is what prompted me to do this but I can't remember exactly what, sorry :/ @ThorvaldLarsen do you remember? i guess you could play on the test setup but otherwise it seems like there should be something which makes sure people set ints as things other than ints don't make sense here. I couldn't remember what the best way to do this between validator, val_mapping, set_parser etc was so I just made it so the bug stopped...

@ThorvaldLarsen
Copy link
Contributor

the problem with the awg driver was when I wanted to step the sequence number with sequence_pos in do1d(). do1d seems to pass floats while the sequence_pos expected an integer hence throwing an error.

To be used for parameters that may be set as floats but should be cast to int
@jenshnielsen
Copy link
Collaborator Author

I made this a bit safer by adding a validator that will check if a float is close to an int and pass in such cases. This can be used together with the set_parser.

I also changed the set parser to int(round(x)) in case what you are trying to set the value to is just under the value. int(x) will always "round" down when it casts

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 like it. We can have endless discussions on whether 1e-5 is "close" or 1e-6 is better, but since this is arbitrary in the end, we might as well use 1e-5. It's a sane value.

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.

And by that, I meant to approve of this PR.

@WilliamHPNielsen WilliamHPNielsen merged commit 54193e6 into microsoft:master Nov 3, 2017
@jenshnielsen jenshnielsen deleted the awg_set_parser branch November 3, 2017 12:44
giulioungaretti pushed a commit that referenced this pull request Nov 3, 2017
Author: Jens Hedegaard Nielsen <[email protected]>

    set_parser for seq element setting added (#832)
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.

6 participants