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

Added condition for different schedulers #691

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

Om-Doiphode
Copy link
Contributor

I have updated the configure_optimizers in the deepforest/main.py file to account for the different learning rate schedulers and also made the necessary changes in the deepforest_config.yml file.

image

@bw4sz
Copy link
Collaborator

bw4sz commented Jun 18, 2024

Great thanks for this PR.

couple pieces

  1. With every pull request, you want to write a pytest that asserts the new functionality. Something like here

    def test_train_no_validation(m):

    but where you change the config, using config_args when creating the deepforest object (https://deepforest.readthedocs.io/en/latest/ConfigurationFile.html#passing-config-arguments-at-runtime-using-a-dict) to show that the changes don't have unexpected consequences.

  2. This change would be breaking for anyone with a previous version of a config file. Copy your new config to where it gets distributed with the rest of the package. @henrykironde this one of those long term maintenance things we want to automate on push, we should have two spots to update config files. But for now, its fine.

deepforest/data/deepforest_config.yml

  1. Its good to have some arg defaults in case users have old custom configs. I think the existing code should stay in the code base above the new code as a default until a major release and mark it as to be deprecated.
https://github.com/Om-Doiphode/DeepForest/blob/6e959757f9bfac578031234b72ca2f4c6097efd9/deepforest_config.yml#L37

@bw4sz bw4sz self-requested a review June 18, 2024 14:38
@Om-Doiphode
Copy link
Contributor Author

HI @bw4sz, I have included 7 test functions in the test_main.py file to account for the different schedulers, updated the deepforest_config.yml files in other folders, and updated the documentation for the scheduler. I have run all the tests, and they have passed successfully.

image

@bw4sz
Copy link
Collaborator

bw4sz commented Jun 19, 2024

Great, instead of writing 7 separate tests. Its one test with multiple parameters. Using pytest.mark.parameterize.

For example:

@pytest.mark.parametrize("architecture",["retinanet","FasterRCNN"])

Copy link
Collaborator

@bw4sz bw4sz left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@bw4sz bw4sz merged commit 42d0d26 into weecology:main Jun 20, 2024
2 of 5 checks passed
@bw4sz
Copy link
Collaborator

bw4sz commented Jun 20, 2024

Nice job @Om-Doiphode

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.

2 participants