Skip to content

Add improved version of Hungarian algorithm. - #1140

Merged
copybara-service[bot] merged 1 commit into
google-deepmind:mainfrom
carlosgmartin:improved_hungarian_algorithm
Jan 29, 2025
Merged

Add improved version of Hungarian algorithm.#1140
copybara-service[bot] merged 1 commit into
google-deepmind:mainfrom
carlosgmartin:improved_hungarian_algorithm

Conversation

@carlosgmartin

Copy link
Copy Markdown
Contributor

Follow-up to #1083 (comment).

@vroulet vroulet left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks again @carlosgmartin!
Some suggestions:

  • keep a single file, and name the first algorithm you did as "base_hungarian_algorithm"
  • add in e.g. the new algorithm's docstring a sentence that differentiates the two implementations.
  • Add a test that compares the results between the two implementations maybe

Otherwise it's great

@carlosgmartin

carlosgmartin commented Nov 21, 2024

Copy link
Copy Markdown
Contributor Author

@vroulet Seems to me it’s cleaner to keep each implementation in a separate file, since

  1. It keeps the implementations less entangled and keeps their respective helper functions separate, avoiding potential confusion.
  2. It keeps the modules shorter/less crowded, especially if more implementations are added in the future.

WDYT?

@vroulet

vroulet commented Nov 22, 2024

Copy link
Copy Markdown
Collaborator

It keeps the implementations less entangled and keeps their respective helper functions separate, avoiding potential confusion.

In any case we should add in the docstring why they differ.

It keeps the modules shorter/less crowded, especially if more implementations are added in the future.

I would actually agree with you, but typically the alias file in optax has an ever increasing list of optimizers. So let's keep the same logic here. (That would actually be my main argument in favor of keeping them in a single file: it may be better for the package to follow a similar philosophy across folders/files).

@carlosgmartin
carlosgmartin force-pushed the improved_hungarian_algorithm branch from 64cdc1d to f134068 Compare November 23, 2024 20:21
@carlosgmartin

Copy link
Copy Markdown
Contributor Author

@vroulet Done.

@vroulet vroulet left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks again @carlosgmartin !

Comment thread optax/_src/alias.py Outdated
@carlosgmartin
carlosgmartin force-pushed the improved_hungarian_algorithm branch from f134068 to b45b76c Compare January 25, 2025 00:41
@copybara-service
copybara-service Bot merged commit 225a707 into google-deepmind:main Jan 29, 2025
@carlosgmartin
carlosgmartin deleted the improved_hungarian_algorithm branch January 29, 2025 18:06
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