Doc revamp for optical flow models #5895
Conversation
There was a problem hiding this comment.
@NicolasHug Thanks, the changes look good.
How would you feel about adding in the metrics of each weights a single epe field that stores the value of the "most appropriate" experiment. Note that this idea implicitly exists already in all other models. For example the acc@1 usually reports the value on the validation set used for training; there are others we could be reporting (or example ImageNet Real/V2 for classification) but currently dont. This way you can have a single metric that summarizes the model metrics and move the per-dataset breakdown on a separate list. I've provided some examples of this previously at #5859 (comment)
We don't have to solve this now but we need to fix this before the release to avoid BC breakages on the future.
|
Perhaps I'm not understanding what you mean. But it seems that this "most appropriate epe" value would be computed on different datasets for each weights, which makes them incomparable with one-another. Reporting them on a single table would be very misleading? |
|
@NicolasHug My remark is not related to reporting them on a single table; I agree this would be misleading and it's exactly why we want to separate Keypoint RCNN from the rest of the detection models at #5882. My comment is about having a single key in the metrics that people can look up for all weights. This is aligned with the idea of multi-weight support API that allows providing weights trained on different datasets. Different weights can be trained on different datasets but they all use the same Acc/F1/Recall/Precision keys. An additional need exists for potentially offer breakdowns on multiple datasets (see for example this paper, page 12 where they train on ImageNet1k but also report on Real and V2). This is very similar to what you have here. So I think we need to finalize the schema for both use-cases. |
|
@NicolasHug I dismissed the review due to the conflicts with main. It's also worth checking the |
| ``epe`` is the "end-point-error" and indicates how far (in pixels) the | ||
| predicted flow is from its true value. This is averaged over all pixels | ||
| of all images. ``per_image_epe`` is similar, but the average is different: | ||
| the epe is first computed on each image independently, and then averaged | ||
| over all images. This corresponds to "Fl-epe" (sometimes written "F1-epe") | ||
| in the original paper, and it's only used on Kitti. ``fl-all`` is also a | ||
| Kitti-specific metric, defined by the author of the dataset and used for the | ||
| Kitti leaderboard. It corresponds to the average of pixels whose epe is | ||
| either <3px, or <5% of flow's 2-norm. |
There was a problem hiding this comment.
I wanted this to be a ..note :: but it looks like it doesn't render properly. It's likely because we treat these docstrings are treated differently from the rest.
There was a problem hiding this comment.
LGTM, thanks @NicolasHug. Just one question for your attention.
| metrics = meta.pop("_metrics") | ||
| for dataset, dataset_metrics in metrics.items(): | ||
| for metric_name, metric_value in dataset_metrics.items(): | ||
| metric_name = metric_name.replace("_", "-") |
There was a problem hiding this comment.
Should we do this? It's the documentation that shows the actual name of the variable. (I know you made it private but still)
|
@NicolasHug Do you also want to do the honours of clipping the old models.srt and replacing it with the new one? This I think is the "last" commit around docs. cc @jdsgomes who is concurrently updating the models.srt on the Swin PR. |
|
Hey @NicolasHug! You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py |
Summary: * Doc revamp for optical flow models * Some more Reviewed By: NicolasHug Differential Revision: D36760919 fbshipit-source-id: b43d45ec9fe9d3a1663a341ef6dec746c7bb1ace
This PR adds docs for the optical flow models, as part of the doc revamp.
Unlike for other sections, I didn't add a table of weight accuracies in the main models page. The reason is that most weights are evaluated on different datasets:
So if we were to build a table from that we would need 7 columns for the metrics, and most values of the table would be empty. I don't think we'll manage to make it look pretty.
What I would suggest to do instead is to add some narrative docs that explain the difference between C_T, C_T_SKHT, and C_T_SKHT_K, and also explain what each of the metrics are. Then users would be able to look up each metric in the builders' docs.
Happy to discuss more though