-
Notifications
You must be signed in to change notification settings - Fork 333
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
Add Sentinel2_CDL Datamodule #1889
Add Sentinel2_CDL Datamodule #1889
Conversation
Are there any changes required to merge this datamodule? |
Also need to add this to docs |
Needs same changes as NCCM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good now. Look through the commits I added and make sure they all make sense. Couple final comments worth reviewing before we meet. I'll try to get the other PRs in similar shape so we can merge after our meeting today
This reverts commit 859f24e.
@adamjstewart Thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks fine, but I want to make sure that you have actually tried to train a model with this!
**kwargs: Any, | ||
) -> None: | ||
"""Initialize a new Sentinel2CDLDataModule instance. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give an example of how to instantiate this class? It is non-obvious from the current docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The datamodule can be initialized like this:
datamodule = Sentinel2CDLDataModule(
crs="epsg:3857",
batch_size=64,
patch_size=224,
cdl_paths="data/cdl/",
sentinel2_paths="data/sentinel2/",
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, can you add that to the docstring?
Returns: | ||
A matplotlib Figure with the image, ground truth, and predictions. | ||
""" | ||
return self.cdl.plot(*args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this plot work when using this with a SemanticSegmentation trainer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's also tested in CI. It simply overrides the default that calls self.dataset.plot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still a few details we need to work out for our paper experiments w.r.t. augmentations, but we can do that in a mass PR once all data modules are merged.
I agree with @calebrob6 that the documentation could always be improved, but this is also true for our existing datamodules, none of which give examples for instantiation. The foo_
prefix stuff is already used in some of our existing datamodules like NAIPChesapeake with no explanation. Let's make a tutorial for these someday that gives example usage. For now, I'll force merge this so we aren't holding up all other datamodule PRs.
) | ||
|
||
self.train_aug = AugmentationSequential( | ||
K.Normalize(mean=self.mean, std=self.std), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder that we still need to figure this out for all data modules in a future PR
Requests are partially resolved, better tutorials can be a future goal
This PR adds the datamodule for CDL + Sentinel-2 images.