-
Notifications
You must be signed in to change notification settings - Fork 3
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
return selected indices #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @oddoking, thanks for the PR!
The change sounds good, can you also update the typing of Simulator.select?
Thank you
chromax/functional.py
Outdated
@@ -142,7 +142,7 @@ def select( | |||
population: Population["n"], | |||
k: int, | |||
f_index: Callable[[Population["n"]], Float[Array, "n"]], | |||
) -> Population["k"]: | |||
) -> (Population["k"], Int[Array, "k"]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change (Population["k"], Int[Array, "k"])
to be Tuple[Population["k"], Int[Array, "k"]]
, where Tuple is imported from typing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@younik cool, updated as your suggest. What do you think now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oddoking looks good, thanks!
There is a small typo in the doc + can you also update the typing of Simulator.select
accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also tests, should be updated accordingly (basically, every time there is pop = simulator.select(...)
, we should change to pop, _ = simulator.select(...)
to unpack args
chromax/functional.py
Outdated
@@ -154,8 +154,8 @@ def select( | |||
(n, m, 2) and returns an array of n float number. | |||
:type f_index: Callable | |||
|
|||
:return: output population of (k, m, d) | |||
:rtype: ndarray | |||
:return: output population of (k, m, d), output indecies of (k,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small typo indices
@younik thanks for the help. I push the change to fix typo and tests. |
Looks good! Many thanks @oddoking for contributing! |
We would like to return the selected indecies as well when doing select with top K. Please have a look