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

Module model structure #476

Closed
wants to merge 1 commit into from
Closed

Module model structure #476

wants to merge 1 commit into from

Conversation

bw4sz
Copy link
Collaborator

@bw4sz bw4sz commented Sep 24, 2023

The goal of this PR is make a Model() class that is independent of the current retinanet structure. This would allow us, and users, to swap in and out the models based on developments in computer vision. As long as they pass a check_input() and check_output() function to match with upstream and downstream workflows. Here is a roadmap

  • Create a model.py in deepforest/ and move retinanet.py to a new models/ directory.
  • Update tests to reflect the change in name
  • Write validation tests that run on model.Model.init() to ensure that the model matches expected shapes and formats.
  • Test new workflow by adding a new model, probably RCNN from torchvision
  • Write a doc on model creation
  • Confirm will download_release work if config and download architecture are different, does it correctly overwrite it?

One of the outstanding issues here is how to handle the parameters specific to each model. My initial idea is to have them as nested parts of the .yml

# Model Architecture
architecture: 'retinanet'
num_classes: 1

# Architecture specific params
retinanet:
    # Non-max supression of overlapping predictions
    nms_thresh: 0.05
    score_thresh: 0.1

which means that when we create_model(), we pass the config. The create_model specific to each model in models/ then looks for the appropriate model params

        model = RetinaNet(backbone=backbone, num_classes=self.config["num_classes"])
        model.nms_thresh = self.config["retinanet"]["nms_thresh"]
        model.score_thresh =  self.config["retinanet"]["score_thresh"]

Pros is that its clean and scalable. Cons its not that readable and explicit, since you can pass the config without knowing whether all the arguments are there.

This relates to a more global problem of being able to specify and pass kwargs through the main module. It would be nice at runtime to be able to either supply a named arg

retinanet.retinanet(nms_thresh=0.1)

or a config file

retinanet.retinanet(config=config)

or both, and have the named args supercede. I will leave this idea until we have achieved the core goals of this PR.

Issue was briefly tagged as #396

@bw4sz bw4sz added this to the DeepForest 2.0 milestone Sep 24, 2023
@bw4sz
Copy link
Collaborator Author

bw4sz commented Sep 27, 2023

There are a couple features specific to retinanet that we need to build into a more general purpose model idea. The limiting by score threshold and NMS filtering might need to happen outside of the model predict function. They are useful tools, but shouldn't be coupled to the predictions.

@bw4sz bw4sz self-assigned this Sep 27, 2023
@bw4sz
Copy link
Collaborator Author

bw4sz commented Sep 28, 2023

@ethanwhite I believe this is ready for your review.

@bw4sz bw4sz changed the title [WIP] Module model structure Module model structure Sep 28, 2023
@bw4sz
Copy link
Collaborator Author

bw4sz commented Oct 7, 2023

sigh, i corrupted this pull request somehow. Will need to be remade and reopened.

@bw4sz bw4sz closed this Oct 7, 2023
@bw4sz bw4sz deleted the model_agnostic branch October 9, 2023 16:34
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.

1 participant