Skip to content

Adds EuroSAT to the list of supported datasets #5114

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 23 commits into from
Jan 18, 2022
Merged

Adds EuroSAT to the list of supported datasets #5114

merged 23 commits into from
Jan 18, 2022

Conversation

frgfm
Copy link
Contributor

@frgfm frgfm commented Dec 19, 2021 •
edited
Loading

Following up on #5108, this PR introduces the following modifications:

  • adds the EuroSAT dataset to the list of supported datasets (please note that there is no train/test split)
  • adds a corresponding unittest
  • adds the corresponding entry in the documentation

Any feedback is welcome!

cc @pmeier

@facebook-github-bot
Copy link

facebook-github-bot commented Dec 19, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit fad24b8 (more details on the Dr. CI page):

✅ None of the CI failures appear to be your fault 💚

🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed yet.

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

pmeier
pmeier reviewed Dec 19, 2021
View reviewed changes
Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Thanks a lot @frgfm for following up so quickly. The PR looks good. I only have some minor suggestions and a few questions inline.

@pmeier pmeier mentioned this pull request Dec 19, 2021
Closed
14 tasks
@frgfm frgfm requested a review from pmeier December 20, 2021 12:30
pmeier
pmeier reviewed Dec 20, 2021
View reviewed changes
Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Looking good. A small nits inline. Other than that, I propose we use at least a somewhat standardized naming scheme for the folders. I would use base_folder as the root folder plus the dataset name and data_folder as the folder that gets extracted from the archive. For an example, have a look at #5115.

pmeier
pmeier approved these changes Dec 21, 2021
View reviewed changes
Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot @frgfm!

@pmeier pmeier requested review from pmeier and NicolasHug and removed request for pmeier December 21, 2021 10:55
NicolasHug
NicolasHug reviewed Jan 5, 2022
View reviewed changes
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks a lot @frgfm for the PR and @pmeier for the review.

I have a few comments / questions below. I understand it's been 2 weeks since the last approval already, so I'm happy to address these myself if you'd like. Let me know!

@pmeier
Copy link
Collaborator

pmeier commented Jan 6, 2022

@frgfm the test failures are related. Could you have a look?

@frgfm
Copy link
Contributor Author

frgfm commented Jan 6, 2022

@frgfm the test failures are related. Could you have a look?

Oops, I overlooked that one. This should be fixed now 👍

@pmeier pmeier requested a review from NicolasHug January 6, 2022 15:23
pmeier
pmeier approved these changes Jan 17, 2022
View reviewed changes
Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Thanks a lot for sticking to it @frgfm! LGTM if CI is green.

NicolasHug
NicolasHug approved these changes Jan 18, 2022
View reviewed changes
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks a lot @frgfm and @pmeier !

@NicolasHug NicolasHug merged commit 57a77c4 into pytorch:main Jan 18, 2022
@frgfm frgfm deleted the eurosat branch January 18, 2022 12:51
facebook-github-bot pushed a commit that referenced this pull request Jan 19, 2022
@pmeier @facebook-github-bot
[fbsync] Adds support for EuroSAT dataset (#5114)
810da58 
Summary:
* feat: Added EuroSAT dataset

* test: Added unittest

* docs: Improved comments

* docs: Updated the documentation

* docs: Removed unnecessary comments

* fix: Fixed class implementation

* test: Fixed unittest

* fix: Fixed magic method len

* test: Fixed unittest

* refactor: Refactored EuroSAT

* refactor: Applied modifications

* Apply suggestions from code review

* refactor: Applied request changes

* refactor: Made var explicit

* fix: Fixed attribute initialization order

* refactor: Removed name mapping

Reviewed By: datumbox, NicolasHug

Differential Revision: D33655260

fbshipit-source-id: 0986a3914c8ad89d3fc54792e53e64f420331746

Co-authored-by: Philip Meier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasHug NicolasHug NicolasHug approved these changes

@pmeier pmeier pmeier approved these changes

Assignees
No one assigned
Labels
ciflow/default cla signed module: datasets new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@frgfm @facebook-github-bot @pmeier @NicolasHug