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

179-add-hierarchical-croptype-picker #191

Merged
merged 6 commits into from
Oct 16, 2024

Conversation

cbutsko
Copy link

@cbutsko cbutsko commented Oct 15, 2024

Possible TODOs for separate branches:

  • introduce a minimum threshold for class count
  • add suggestion on how to proceed if too few classes are found (e.g. "Select bigger window", or "Increase the buffer size")
  • add select/deselect all button for lower level
  • guide to the legend description

@cbutsko cbutsko requested a review from kvantricht October 15, 2024 15:03
@cbutsko cbutsko linked an issue Oct 15, 2024 that may be closed by this pull request
4 tasks
@cbutsko cbutsko requested review from jdegerickx and removed request for kvantricht October 15, 2024 15:04
Copy link
Contributor

@jdegerickx jdegerickx left a comment

Choose a reason for hiding this comment

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

Instead of reviewing the code itself, I just tried it myself in a notebook. Seemed to work fine, except for the samples_threshold...

Some observations:

image
--> when I only select maize among the cereals class, I intuitively expected all other cereal classes would get grouped into an "other cereals" class. Instead, these cereals get merged into "other_temporary_crops". I guess this is because we do not support hierarchical crop type models (yet). Fine with me, we should just document this very clearly in the notebook...

  • samples threshold did not seem to work correctly?
    image

  • In its current state, a user can train a model with 2 classes, one containing 1000 samples and the other containing 10. Shouldn't we protect the user from such models? And perhaps implement a minimum sample amount per class?

@kvantricht kvantricht self-requested a review October 16, 2024 15:15
@kvantricht kvantricht merged commit a49aac2 into main Oct 16, 2024
4 checks passed
@kvantricht kvantricht deleted the 179-add-hierarchical-croptype-picker branch October 16, 2024 15:17
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.

Add hierarchical croptype picker
3 participants