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

segmentation and depth ds #8

Merged
merged 14 commits into from
Aug 25, 2020
Merged

segmentation and depth ds #8

merged 14 commits into from
Aug 25, 2020

Conversation

BobrG
Copy link
Collaborator

@BobrG BobrG commented Aug 19, 2020

No description provided.

@BobrG BobrG requested a review from rakhimovv August 19, 2020 18:05
@@ -133,3 +133,117 @@ def __getitem__(self, index):
file_index_to_unload = np.random.choice(loaded_file_indexes)
self.files[file_index_to_unload].unload()
return item

class DepthDataset(LotsOfHdf5Files):
Copy link
Collaborator

@rakhimovv rakhimovv Aug 20, 2020

Choose a reason for hiding this comment

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

what is the point of inheritance if you redefine everything? almost all logic working with hdf5 I suppose is already well implemented inside LotsOfHdf5Files class by @artonson. I suppose DepthDataset should mostly concentrate on data and target preprocessing without diving into hdf5 reading logic


def __getitem__(self, index):

data, target = self._getdata(index)
Copy link
Collaborator

@rakhimovv rakhimovv Aug 20, 2020

Choose a reason for hiding this comment

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

here it is better to do something like this

Suggested change
data, target = self._getdata(index)
data = super().__getitem__(index)

rmse_sum += output['rmse_sum']
size += output['batch_size']
mean_rmse = gather_sum(rmse_sum) / gather_sum(size)
logs = {f'{prefix}_mean_rmse': mean_rmse}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why I see regression metrics inside Segmentation PL module

target = torch.FloatTensor(dist_mask)

data = torch.FloatTensor(data).unsqueeze(0)
data = torch.cat([data, data, data], dim=0)
Copy link
Collaborator

@rakhimovv rakhimovv Aug 20, 2020

Choose a reason for hiding this comment

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

what is this? data = torch.cat([data, data, data], dim=0)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in my version of unet input had to have 3 channels. Should I remove this and keep one channel for input data?

Copy link
Collaborator

@rakhimovv rakhimovv Aug 20, 2020

Choose a reason for hiding this comment

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

remove. Unet in our reference implementation https://github.com/qubvel/segmentation_models.pytorch/blob/master/segmentation_models_pytorch/unet/model.py#L52 allows to setup in_channels param.

Comment on lines 225 to 234
mask_1 = (np.copy(data) != 0.0).astype(float) # mask for object
mask_2 = np.where(data == 0) # mask for background

data = self.quantile_normalize(data)
data = self.standartize(data)

dist_new = np.copy(target)
dist_mask = dist_new * mask_1 # select object points
dist_mask[mask_2] = 1.0 # background points has max distance to sharp features
close_to_sharp = np.array((dist_mask != np.nan) & (dist_mask < 1.)).astype(float)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose some of these preprocessing steps are task-specific and should be stated inside if closure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

then I suppose it's better to add a normalisation parameter to cfg file and dataset init. This parameter would be a list of specific preprocessing methods to apply

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, could be a possible solution

data = torch.FloatTensor(data).unsqueeze(0)
data = torch.cat([data, data, data], dim=0)

return data, target
Copy link
Collaborator

@rakhimovv rakhimovv Aug 20, 2020

Choose a reason for hiding this comment

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

The better idea would be to return a dictionary instead of a tuple, so there is no ambiguity then what is a target

@rakhimovv
Copy link
Collaborator

The good point is also to try running code in the first place before committing

@BobrG
Copy link
Collaborator Author

BobrG commented Aug 20, 2020

@rakhimovv please see new changes though I haven't run code yet

from torch.utils.data import DataLoader

from sharpf.utils.comm import get_batch_size
from sharpf.utils.losses import balanced_accuracy
Copy link
Collaborator

@rakhimovv rakhimovv Aug 20, 2020

Choose a reason for hiding this comment

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

I haven't found the implementation of balanced_accuracy

mean_rmse = gather_sum(rmse_sum) / gather_sum(size)
logs = {f'{prefix}_mean_rmse': mean_rmse}
return {f'{prefix}_mean_rmse': mean_rmse, 'log': logs}
mean_metric = gather_sum(metric_sum) / gather_sum(size)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the balanced accuracy needs a bit different gathering logic across batches

@rakhimovv
Copy link
Collaborator

rakhimovv commented Aug 20, 2020

Also, I do not see any changes in DepthDataset. Maybe you forgot to git add. Please first run code without crashes, then commit. For faster debugging you can set command line arg trainer.fast_dev_run=true

@BobrG
Copy link
Collaborator Author

BobrG commented Aug 20, 2020

added depth ds changes


return data, target
return {'data': data, 'target': target}
Copy link
Collaborator

@rakhimovv rakhimovv Aug 20, 2020

Choose a reason for hiding this comment

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

I meant something more informative :) like {'is_sharp_mask': ..., 'image': ...} or {'sharpness': ..., 'image': ...}. So the keys inside dict represent the task

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so then keys should differ for each task? what's the purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or they should be similar for each task in depth dataset, just have names which describe what kind of data is this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

for the sake of readability

Copy link
Collaborator

@rakhimovv rakhimovv Aug 20, 2020

Choose a reason for hiding this comment

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

and also for example, if you want to add later several new additional targets, it would be much easier to add them just like new keys: {'image': ..., 'sharpness': ..., 'normals': ..., 'whatever else': ...}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also, for segmentation task for ex. I have a binary close-to-sharp target and for regression a distance field, then there should be several returns for each task or same names

Copy link
Collaborator

Choose a reason for hiding this comment

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

{'image': ..., 'distance_field': ..., 'close_to_sharp_mask': ...}

train_net.py Outdated
@@ -36,7 +38,7 @@ def main(cfg: DictConfig):
log.info(f"Original working directory: {hydra.utils.get_original_cwd()}")
seed_everything(cfg.seed)

model = instantiate(cfg.meta_arch, cfg=cfg)
model = DepthRegressor(cfg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

train_net.py should be model-agnostic as it was

points, distances = batch['image'], batch['distances']
points = points.unsqueeze(1) if points.dim() == 3 else points
preds = self.forward(points)
print(preds.shape, distances.shape)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do not include please debugging output into the commit

@BobrG
Copy link
Collaborator Author

BobrG commented Aug 21, 2020

@rakhimovv are there any other comments on my last commit?

@rakhimovv
Copy link
Collaborator

rakhimovv commented Aug 21, 2020

I still do not see balanced_accuracy implementation. You can google for an example of different segmentation metrics, how they were implemented by PL community. Maybe it was already implemented by somebody. Please make sure that it works in a multi-gpu regime. Check the docs about that here.

Also, check the last changes in pl_hydra branch and merge changes from pl_hydra to segmentation branch (not in the reverse direction, I will do it myself after approving this PR). The major change is due to facebookresearch/hydra#882, i.e. now we use _target_ instead of target and no more params key when instantiating new objects. You can see actual documentation here

@BobrG
Copy link
Collaborator Author

BobrG commented Aug 21, 2020

@rakhimovv please have a look at balanced accuracy
also, do you still have concerns about _shared_eval_step and _shared_eval_epoch_end from depth_segmentator.py ? should I reconsider the logic of metric calculation?

@rakhimovv
Copy link
Collaborator

rakhimovv commented Aug 21, 2020

@BobrG as it works now, you calculate the balanced accuracy on each "sub" batch on each gpu and then average it. Instead, you should output predictions and corresponding targets inside eval step, then gather all predictions and targets on epoch end and finally then calculate the metric. The gathering is necessary because otherwise, you skip data. Please read docs, I attached links above in the previous comment.

@BobrG
Copy link
Collaborator Author

BobrG commented Aug 24, 2020

@rakhimovv can you please have a look at my last commit?

@rakhimovv
Copy link
Collaborator

Good job @BobrG. I will refactor and polish some minor things myself and after that will merge

@rakhimovv rakhimovv merged commit 221d76a into pl_hydra Aug 25, 2020
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.

2 participants