Skip to content

Conversation

@nikitashvarts
Copy link
Member

Massive changes in folders structure, main classes, call logic, input/output data formats.
Therefore, please make a full review of the entire project.
In case of any questions please contact me @nikolaev-n

@nikitashvarts nikitashvarts self-assigned this Dec 16, 2020
@nikitashvarts nikitashvarts changed the base branch from master to develop December 16, 2020 14:19
@mrTahion mrTahion marked this pull request as ready for review December 16, 2020 14:27
@mrTahion mrTahion marked this pull request as draft December 16, 2020 14:30
Copy link
Member

@mrTahion mrTahion left a comment

Choose a reason for hiding this comment

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

Вроде местами не хватает запуска linter.sh

DATA:
FEAT_EMB:
PATH: "data/CUB/resnet101/"
PATH: "datasets/CUB/resnet101/"
Copy link
Member

Choose a reason for hiding this comment

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

Все ещё сомневаюсь насчет этого переименования

Comment on lines +4 to +6
ZeroShotEval is provided with 2 build-in datasest:
- AWA2
- CUB No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Что мешает использовать, например, SUN?

Comment on lines 11 to 13
Please note, that this script is designed for CUB, SUN, AWA1, AWA2
datasets only, and moreover these datasets must be downloaded from:
https://www.dropbox.com/sh/btoc495ytfbnbat/AAAaurkoKnnk0uV-swgF-gdSa?dl=0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Please note, that this script is designed for CUB, SUN, AWA1, AWA2
datasets only, and moreover these datasets must be downloaded from:
https://www.dropbox.com/sh/btoc495ytfbnbat/AAAaurkoKnnk0uV-swgF-gdSa?dl=0
Please note, that this script is designed for CUB, SUN, AWA1, AWA2
.. _datasets:
https://www.dropbox.com/sh/btoc495ytfbnbat/AAAaurkoKnnk0uV-swgF-gdSa?dl=0

Comment on lines 105 to 107
# NOTE: there are two more fields in mat files that are not in use now, but can be used
# train_loc = mat_data['train_loc'].squeeze() - 1 #--> train_feature = TRAIN SEEN
# val_unseen_loc = mat_data['val_loc'].squeeze() - 1 #--> test_unseen_feature = TEST UNSEEN
Copy link
Member

Choose a reason for hiding this comment

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

Может стоит удалить?

from zeroshoteval.utils.parser import load_config, parse_args
from zeroshoteval.zeroshotnets.build import build_zsl

os.chdir("../")
Copy link
Member

Choose a reason for hiding this comment

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

Для чего эта строка?

Comment on lines +1 to +3
import torch
from fvcore.common.config import CfgNode
from torch.utils.data.dataloader import DataLoader
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import torch
from fvcore.common.config import CfgNode
from torch.utils.data.dataloader import DataLoader
from fvcore.common.config import CfgNode
from torch.utils.data.dataloader import DataLoader

Comment on lines +78 to +83
loader = torch.utils.data.DataLoader(
dataset,
batch_size=cfg.ZSL.BATCH_SIZE,
num_workers=cfg.DATA_LOADER.NUM_WORKERS,
pin_memory=cfg.DATA_LOADER.PIN_MEMORY,
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
loader = torch.utils.data.DataLoader(
dataset,
batch_size=cfg.ZSL.BATCH_SIZE,
num_workers=cfg.DATA_LOADER.NUM_WORKERS,
pin_memory=cfg.DATA_LOADER.PIN_MEMORY,
)
loader = DataLoader(
dataset,
batch_size=cfg.ZSL.BATCH_SIZE,
num_workers=cfg.DATA_LOADER.NUM_WORKERS,
pin_memory=cfg.DATA_LOADER.PIN_MEMORY,
)

Args:
idx: Index of sample to return.
Returns: Tuple2 with dictionary of `modalities names: item from each modality` and `sample label`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Returns: Tuple2 with dictionary of `modalities names: item from each modality` and `sample label`
Returns:
sample_data ...
sample_label ...

return len(self.labels)

def __getitem__(self, idx):

Copy link
Member

Choose a reason for hiding this comment

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

docstring?

self._data_loader_iter = iter(data_loader)

def evaluate(self) -> None:
logger = logging.getLogger(__name__)
Copy link
Member

Choose a reason for hiding this comment

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

мб вынести наружу?

return args


def read_data(images_mat_file: str,
Copy link

@termit209 termit209 Dec 18, 2020

Choose a reason for hiding this comment

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

может более понятнее будет название read_data_mat или что-то похожее?

# --------------------------------------------------------------------------

# TODO: extract gen_synthetic_data from CADA-VAE and move it to inference section
pass

Choose a reason for hiding this comment

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

В этом ПР пока отказываемся от идеи вынести емб экстрактор из тренировки?

from zeroshoteval.utils.exceptions import ExtractorTypeError


class ImageEmbeddingExtractor(EmbeddingExtractor):

Choose a reason for hiding this comment

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

Это окончательная версия экстрактора или будет переделываться?



class DecoderTemplate(nn.Module):
"""Decoder part for CADA-VAE model.

Choose a reason for hiding this comment

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

Может в докстринге написать что это просто декодер, так как и лизгане по сути такойже декодер?

self.labels = None

assert split in ["trainval", "test", "test_unseen"]
assert mod in ["CLS_ATTR", "IMG"]

Choose a reason for hiding this comment

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

разве везде не изменили CLS_ATTR на CLSATTR?

Comment on lines 108 to 109
_C.ZSL.EPOCH = 100
# _C.ZSL.EPOCH = 100
_C.ZSL.EPOCH = 10
Copy link
Member

Choose a reason for hiding this comment

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

Для чего это изменение? В любом случае не стоит, кмк, оставлять закоменченные строки.

Comment on lines +31 to +32
train_loader = construct_loader(cfg, split="train")
test_loader = construct_loader(cfg, split="test")
Copy link
Member

Choose a reason for hiding this comment

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

Кмк train_loader и test_loader в инференсе совсем неочевидные названия.

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.

4 participants