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

Malte lig 1262 advanced selection external docs #900

Merged
merged 46 commits into from Sep 8, 2022

Conversation

MalteEbner
Copy link
Contributor

@MalteEbner MalteEbner commented Aug 15, 2022

Description

  • adds docs for selection using the new config
  • adapts existing docs (getting started, docker active learning)
  • already included: DIVERSIFY->DIVERSITY

Tabs vs. Dropdowns

Here tabs are used for the SelectionStrategy and Dropdowns for the configuration examples.
image

@philippmwirth
Copy link
Contributor

As an initial test I went through the "First Steps" section and I have some feedback. Will summarize it below.

Feedback:

  • "Scheduling a Simple Job"
    • I copy pasted the command and got a syntax error (there's a missing comma)
    • After fixing it, it complained that all the imports are missing (-> add imports)
    • Then I had the problem that the imports didn't work because I had the wrong PIP version (-> clarify)
    • Then I got TypeError: __init__() got an unexpected keyword argument 'type' because there's a typo:
           SelectionConfigEntry(
                input=SelectionConfigEntry(type=SelectionInputType.EMBEDDINGS),
                strategy=SelectionConfigEntry(type=SelectionStrategyType.DIVERSIFY)
            )
    should be
           SelectionConfigEntry(
                input=SelectionConfigEntryInput(type=SelectionInputType.EMBEDDINGS),
                strategy=SelectionConfigEntryStrategy(type=SelectionStrategyType.DIVERSIFY)
            )
    • Finally, I managed to run it (it's running in the background now).
  • The typos mentioned above can also be found in "Training a Self-supervised Model"
  • In the example for "Training a Self-supervised Model" with exposed lightly arguments there's no selection config
  • The typos mentioned above can also be found in "Specifying Relevant Files"
  • In the "Selection":
    • I don't think this is the proper way to use the word "tell":

    The input tells on which data the objective is defined on. It defines a scalar number or vector for each image in the dataset.

    • I don't think we support custom embeddings so why is it documented so prominently?
    • The formatting in the "Selection Input" and "Selection Strategy" chapters is strange, just leave the input= away.
    • For the "Configuration Examples": Don't use tabs - it looks really bad.
    • I don't think the object frequency scorer works the way it's documented. As you can see here it's always normalized to [0, 1] so thresholding > 2 doesn't make sense.

@IgorSusmelj please note that some of this feedback might be outdated after the discussion we had with @MalteEbner @guarin and @michal-lightly about how to pass the selection config (dict vs object).

I will provide more feedback when testing a more involved example but in general we need to make sure that all the examples and code snippets work!

@IgorSusmelj
Copy link
Contributor

IgorSusmelj commented Aug 18, 2022

Suggestion for blocks you can test:

  • First steps (/docker/getting_started/first_steps.html) (@philippmwirth)
  • Selection until configuration examples (docker/getting_started/selection.html) (@guarin )
  • Selection from (including) configuration examples to end (docker/getting_started/selection.html) (@philippmwirth)
  • Advanced
    • Datapool + pretagging (/docker/advanced/datapool.html + /docker/advanced/pretagging.html) (@guarin )
    • Active Learning + Object Level (/docker/advanced/active_learning.html + /docker/advanced/object_level.html) (@guarin )
    • Sequence selection (/docker/advanced/sequence_selection.html)(@MalteEbner)
  • Integration (/docker/integration/dagster_aws.html) (@guarin )
  • Examples (/docker/examples/overview.html) (@guarin )

Copy link
Contributor

@philippmwirth philippmwirth left a comment

Choose a reason for hiding this comment

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

Reviewed "First steps (/docker/getting_started/first_steps.html)" so far. Good news is that I only had to fix one thing and everything ran through 🙂

One thing I noticed: The percentage stopping condition is not documented anywhere.

docs/source/docker/getting_started/first_steps.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/first_steps.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/first_steps.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/first_steps.rst Outdated Show resolved Hide resolved
@MalteEbner
Copy link
Contributor Author

One thing I noticed: The percentage stopping condition is not documented anywhere.

Because it is not implemented yet: https://linear.app/lightly/issue/LIG-1661/add-proportion-samples-to-openapi-specs

Copy link
Contributor

@guarin guarin left a comment

Choose a reason for hiding this comment

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

Partial review for the getting_started/selection.rst part. Will finish it tomorrow :)

I tried the code and it worked (with some minor tweaks). I'll also try all the "configuration examples" later on.

Edit: It looks like @philippmwirth already tested the remaining part so I'll skip it.

docs/source/docker/integration/examples/trigger_job.py Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/integration/examples/trigger_job.py Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@philippmwirth philippmwirth left a comment

Choose a reason for hiding this comment

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

Reviewed the "Getting Started -> Selection" part. All the tests run through when switching n_samples to nSamples but I have some more feedback.

docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@guarin guarin left a comment

Choose a reason for hiding this comment

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

Next review :)

The code worked like a charm 💯

docs/source/docker/advanced/datapool.rst Outdated Show resolved Hide resolved
docs/source/docker/advanced/pretagging.rst Outdated Show resolved Hide resolved
docs/source/docker/advanced/pretagging.rst Outdated Show resolved Hide resolved
docs/source/docker/advanced/pretagging.rst Outdated Show resolved Hide resolved
docs/source/docker/advanced/active_learning.rst Outdated Show resolved Hide resolved
docs/source/docker/advanced/active_learning.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@guarin guarin left a comment

Choose a reason for hiding this comment

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

Final review 😅

@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #900 (3756e66) into master (aafdc2f) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #900      +/-   ##
==========================================
+ Coverage   89.38%   89.41%   +0.02%     
==========================================
  Files          98       98              
  Lines        4429     4439      +10     
==========================================
+ Hits         3959     3969      +10     
  Misses        470      470              
Impacted Files Coverage Δ
lightly/api/api_workflow_compute_worker.py 100.00% <ø> (ø)
lightly/api/api_workflow_datasources.py 98.70% <0.00%> (+0.19%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@MalteEbner MalteEbner force-pushed the malte-lig-1262-advanced-selection-external-docs branch from 707eb17 to 25a3c22 Compare August 25, 2022 14:30
@MalteEbner MalteEbner force-pushed the malte-lig-1262-advanced-selection-external-docs branch from d13b85a to 747a8cb Compare August 26, 2022 12:47
Copy link
Contributor

@guarin guarin left a comment

Choose a reason for hiding this comment

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

Great work! Almost there! There are a few typos and minor errors left but otherwise this looks great 💯

docs/source/docker/advanced/active_learning.rst Outdated Show resolved Hide resolved
docs/source/docker/advanced/active_learning.rst Outdated Show resolved Hide resolved
docs/source/docker/advanced/active_learning.rst Outdated Show resolved Hide resolved
docs/source/docker/examples/academic_datasets.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
docs/source/docker/getting_started/selection.rst Outdated Show resolved Hide resolved
@MalteEbner
Copy link
Contributor Author

MalteEbner commented Sep 5, 2022

I removed the whole "active_learning.rst" part as

  • we do not have a specific active learning workflow anymore, it is part of the general selection
  • it is still in the docker archive for those that need it

@philippmwirth
Copy link
Contributor

I removed the whole "active_learning.rst" part as

* we do not have a specific active learning workflow anymore, it is part of the general selection

* it is still in the docker archive for those that need it

This will result in a dead link which is not optimal. Can you add a redirect to the selection page at least?

@MalteEbner
Copy link
Contributor Author

MalteEbner commented Sep 5, 2022

I removed the whole "active_learning.rst" part as

* we do not have a specific active learning workflow anymore, it is part of the general selection

* it is still in the docker archive for those that need it

This will result in a dead link which is not optimal. Can you add a redirect to the selection page at least?

I added a redirect:

From the logs:

::1 - - [05/Sep/2022 16:04:49] "GET /docker/advanced/active_learning.html HTTP/1.1" 200 -
::1 - - [05/Sep/2022 16:04:49] "GET /docker/getting_started/selection.html HTTP/1.1" 200 -

Please be aware that docker/advanced will NOT include active learning anymore, as this will be part of the regular selection.

Copy link
Contributor

@philippmwirth philippmwirth left a comment

Choose a reason for hiding this comment

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

I have a bit more feedback.

I also have a question: Do you know if @guarin tested out all the examples?

@MalteEbner
Copy link
Contributor Author

I also have a question: Do you know if @guarin tested out all the examples?

I assume that he has, as there are ticks for all the tests where he put his name.

Copy link
Contributor

@michal-lightly michal-lightly left a comment

Choose a reason for hiding this comment

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

Just approving since other people are OOO.

@MalteEbner MalteEbner merged commit f55e3f9 into master Sep 8, 2022
@MalteEbner MalteEbner deleted the malte-lig-1262-advanced-selection-external-docs branch September 8, 2022 08:16
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.

None yet

6 participants