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 range(stop) with default start for override grammar #1664

Merged
merged 2 commits into from
Jun 23, 2021

Conversation

victorjoos
Copy link
Contributor

Motivation

To make the range function more like the default python range(), and easier to type, I added a check if only start was added to set stop to start and start to zero.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

tested by adding a parametrize argument in test_basic_sweeper.py.

Related Issues and PRs

N/A

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 10, 2021
@victorjoos victorjoos marked this pull request as ready for review June 10, 2021 09:12
@Jasha10
Copy link
Collaborator

Jasha10 commented Jun 10, 2021

Thanks for the PR!

It would be good to have a few more tests -- see the tests/test_overrides_parser.py file, where most of the range tests currently reside.
In particular, I think it would be good to pin down behavior for the following:

  • behavior when some inputs are zero or negative, e.g. range(-10, step=1), range(10, step=-1), range(0), ...
    for some of these cases, it is not immediately obvious to me what the behavior should be.
  • behavior when called on float inputs

@victorjoos victorjoos marked this pull request as draft June 10, 2021 13:12
@victorjoos
Copy link
Contributor Author

Aaah, Thanks for the tips !

I was a bit fast in implementing this, didn't notice I had to also modify the parser. Let me modify the right files.

Not sure about the float inputs, but for the other cases I would say the best is to compare to the default python range implementation:

  • range(-10) == []
  • range(-2, step=-1) == [0, -1]
  • range(0) == []
  • range(10, step=-1) == []

@victorjoos victorjoos marked this pull request as ready for review June 10, 2021 13:49
@victorjoos
Copy link
Contributor Author

Actually didn't need to change the parser. I added some tests and a new example in the docs.

Copy link
Contributor

@jieru-hu jieru-hu left a comment

Choose a reason for hiding this comment

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

one nit, otherwise lgtm. Thanks!

news/1664.feature Outdated Show resolved Hide resolved
@jieru-hu jieru-hu requested a review from omry June 10, 2021 17:14
@jieru-hu
Copy link
Contributor

@omry could you take a look as well?

@omry
Copy link
Collaborator

omry commented Jun 10, 2021

Of course. but as I am in the middle of releasing 1.1 this will wait.

Copy link
Collaborator

@omry omry left a comment

Choose a reason for hiding this comment

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

Thanks. Looking pretty good.
See comments.

tests/test_basic_sweeper.py Show resolved Hide resolved
tests/test_overrides_parser.py Show resolved Hide resolved
hydra/_internal/grammar/grammar_functions.py Show resolved Hide resolved
news/1664.feature Outdated Show resolved Hide resolved
@victorjoos
Copy link
Contributor Author

Thank you, sorry for the delay. I implemented your suggestions.

Copy link
Collaborator

@omry omry left a comment

Choose a reason for hiding this comment

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

Looking good, thanks.

@omry
Copy link
Collaborator

omry commented Jun 21, 2021

We have some existing CI failures we are looking at. Will likely need you to rebase once those are fixed to get CI green again.

@jieru-hu
Copy link
Contributor

@victorjoos could you rebase master and force push to this branch? Many thanks!

@jieru-hu jieru-hu merged commit 96b3a25 into facebookresearch:master Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants