Niftireader #1151

ssktotoro · 2021-03-31T04:21:08Z

Before submitting (checklist)

Description

Related Issue

Type of Change

Examples / docs / tutorials / contributors update
Bug fix (non-breaking change which fixes an issue)
Improvement (non-breaking change which improves an existing feature)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

PS

I know, that I could join slack for pull request discussion.

Scitator · 2021-03-31T05:37:14Z

Thanks for the PR! Could you please also:

add a new class to the docs, like https://github.com/catalyst-team/catalyst/blob/master/docs/api/data.rst#readers
add new requirements to github CI codestyle and checks
as far as you have >= requirements we should also add it to the minimal requirements here

PS. would you like to contribute any extra test for NiftyReader for the community?

ssktotoro · 2021-03-31T14:50:44Z

is there a test for existing readers like the cv reader?

Scitator · 2021-03-31T15:57:17Z

Not sure if :)
(our main contrib issue)

But you could use our utils or data test for inspiration.

ssktotoro · 2021-03-31T18:03:00Z

@Scitator not really sure how to fix the cache-pip issue >.<
I don't know how the .github/workflows/codestyle.yml works

Scitator · 2021-04-01T05:34:47Z

catalyst/contrib/data/nifti/reader.py

@@ -0,0 +1,39 @@
+from typing import Optional
+
+import nibabel as nib


Suggested change

import nibabel as nib

from catalyst.settings import SETTINGS

if SETTINGS.nifti_required:

import nibabel as nib

by this way, we could build the documentation even without the nibabel package :)

Scitator · 2021-04-01T05:39:55Z

Looks like you have change the reviewdog workflow. Nevertheless, we also need to add requirements update: cache and installation to the codestyle-and-docs one. That's the case :)

Scitator · 2021-04-01T16:46:16Z

.github/workflows/codestyle.yml

+        uses: actions/cache@v2
+        with:
+          path: ${{ steps.pip-cache.outputs.dir }}
+          key: ${{ runner.os }}-${{ matrix.python-version }}-pip -${{ hashFiles('./requirements/requirements-cv.txt') }} -${{ hashFiles('./requirements/requirements-hydra.txt') }} -${{ hashFiles('./requirements/requirements-ml.txt') }} -${{ hashFiles('./requirements/requirements-nifti.txt') }} -${{ hashFiles('./requirements/requirements-optuna.txt') }} -${{ hashFiles('./requirements/requirements-mlflow.txt') }}


I think we need to change the key in case of several caches

can you point me to either what consumes codestyle.yml so I know what is valid/ good for the yml?

Or you can suggest directly what I'm supposed to change. I don't know what consumes the yml and what the key even means and therefore don't know what to change the key to...

honestly speaking, I suggest just return the initial case with united pip cache
I need an extra time to check GitHub CI docs, so we could update the cache logic later

I can do that... just fyi the test will fail b/c the key in the initial case is too long.

.github/workflows/codestyle.yml

Scitator

could we do so?

.github/workflows/codestyle.yml

Pull request has been modified.

ssktotoro · 2021-04-01T18:58:58Z

The test makes CI fail as the requirements for contrib are presumably not installed. Not sure what to do about it.

Scitator

could that help?

Scitator · 2021-04-01T19:10:37Z

.github/workflows/codestyle.yml

+        uses: actions/cache@v2
+        with:
+          path: ${{ steps.pip-cache.outputs.dir }}
+          key: ${{ runner.os }}-${{ matrix.python-version }}-pip-contrib -${{ hashFiles('./requirements/requirements-cv.txt') }} -${{ hashFiles('./requirements/requirements-hydra.txt') }} -${{ hashFiles('./requirements/requirements-ml.txt') }} -${{ hashFiles('./requirements/requirements-nifti.txt') }} -${{ hashFiles('./requirements/requirements-optuna.txt') }} -${{ hashFiles('./requirements/requirements-mlflow.txt') }}
          restore-keys: |
            ${{ runner.os }}-${{ matrix.python-version }}-pip-


Suggested change

${{ runner.os }}-${{ matrix.python-version }}-pip-

${{ runner.os }}-${{ matrix.python-version }}-pip-contrib

.github/workflows/codestyle.yml

ssktotoro · 2021-04-01T22:06:16Z

don't know why catalyst / dl-cpu (ubuntu-20.04, 3.7, minimal) (pull_request) fails, but it should be good?

Scitator · 2021-04-02T04:54:31Z

that should be okay – Github CI freezes sometimes

* changes for NiftiReader * adding import statement that was needed and fixing codestyle * updated CHANGELOG * removing unnecesary documentation * fixing codestyle * updated docs and requirements * adding a space * not importing nibabel for docs * fixing typo * adding step for contrib pip caching * fixed codestyle and added a test for reader that uncovered a mistake :) * hopefully fixing imports * adding sergey suggested fix * fixing deploy_push.yml * updating workflows Co-authored-by: Kevin Wang <kevin.wang@affectiva.com> Co-authored-by: Kevin Wang <kcwang26@gsu.edu>

* add movielens * First hitrate metric version * metrge with upstream * fix topk_map * fix error in map * tests for map * top_k ndcg * edit changelog * edit the tabs * check codestyle * check the intent * check the intent * remove trailing whitespace * fixed hitrate * fixed the docs * hitrate * Additive margin softmax (#1131) * Added Additive-margin softmax * update changelog.md * Readme update2 (#1142) * readme * batch overfit fix for reproducibility * batch overfit fix for reproducibility * readme * readme update * extra Config/Hydra API fixes * codestyle * updated dl_cpu(workflows)- For passing CI-Tests (#1135) * updated dl_cpu * Updated-requirements-ml.txt * corrected the mistake * Update dl_cpu.yml * Update requirements-ml.txt * Kept dl_cpu 20.04-3.6 minimal as requested * Corrected the mistakes * Add requirements parameter * Update dl_cpu.yml * Update dl_cpu.yml Co-authored-by: Sergey Kolesnikov <scitator@gmail.com> * fix: `_key_value` for schedulers in case of multiple optimizers fixed (#1146) * Github CI fixes (#1143) * verbose * Utils callbacks (#1127) * quantization.py * onnx and quantization * tracing.py * docs * fix * fix * quantization test * fix test * fix comments * fix comments * Update catalyst/callbacks/__init__.py Co-authored-by: Sergey Kolesnikov <scitator@gmail.com> * tests * tests * tests * tests * tests * pycharm tricks * extra tests * Utils callbacks (#1145) * quantization.py * onnx and quantization * tracing.py * docs * fix * fix * quantization test * fix test * fix comments * fix comments * quantization example * examples * CHANGELOG.md * fix * fix * Apply suggestions from code review Co-authored-by: Sergey Kolesnikov <scitator@gmail.com> * extra tests 2 * extra tests 2 * extra tests 2 * extra tests 2 * extra tests 2 * extra tests 2 * extra tests 2 * extra tests 2 Co-authored-by: Nikita Balagansky <37884009+elephantmipt@users.noreply.github.com> * Engine docs (#1141) * typings & few docs * typing fix; disabled `dist.barrier()` in optimizer step for ddp * docs * docs: fixed long lines with docs * docs fixes * optimizer args * removed empty line Co-authored-by: Dmytro Doroshenko <dimdoroshenko@gmail.com> * v21.03.1 * v21.03: minimal version fix (#1147) * minimal version fix * docs * fix: nested dicts in loaders_params/samplers_params overriding (#1150) * docs (#1152) * Github CI fix (#1148) * fix * fix * Niftireader (#1151) * changes for NiftiReader * adding import statement that was needed and fixing codestyle * updated CHANGELOG * removing unnecesary documentation * fixing codestyle * updated docs and requirements * adding a space * not importing nibabel for docs * fixing typo * adding step for contrib pip caching * fixed codestyle and added a test for reader that uncovered a mistake :) * hopefully fixing imports * adding sergey suggested fix * fixing deploy_push.yml * updating workflows Co-authored-by: Kevin Wang <kevin.wang@affectiva.com> Co-authored-by: Kevin Wang <kcwang26@gsu.edu> * add changelog * solved runtime error * hitrate calculation Co-authored-by: Daniel Chepenko <dchepenk@yahoo-corp.co> Co-authored-by: denyhoof <kde97@yandex.ru> Co-authored-by: Даниил <zkid18@MacBook-Pro.Dlink> Co-authored-by: Atharva Phatak <athp456@gmail.com> Co-authored-by: Sergey Kolesnikov <scitator@gmail.com> Co-authored-by: Yauheni Kachan <19803638+bagxi@users.noreply.github.com> Co-authored-by: Nikita Balagansky <37884009+elephantmipt@users.noreply.github.com> Co-authored-by: Dmytro Doroshenko <dimdoroshenko@gmail.com> Co-authored-by: Kevin Wang <kcwang@ymail.com> Co-authored-by: Kevin Wang <kevin.wang@affectiva.com> Co-authored-by: Kevin Wang <kcwang26@gsu.edu>

* changes for NiftiReader * adding import statement that was needed and fixing codestyle * updated CHANGELOG * removing unnecesary documentation * fixing codestyle * updated docs and requirements * adding a space * not importing nibabel for docs * fixing typo * adding step for contrib pip caching * fixed codestyle and added a test for reader that uncovered a mistake :) * hopefully fixing imports * adding sergey suggested fix * fixing deploy_push.yml * updating workflows Co-authored-by: Kevin Wang <kevin.wang@affectiva.com> Co-authored-by: Kevin Wang <kcwang26@gsu.edu>

Kevin Wang and others added 2 commits March 30, 2021 22:39

changes for NiftiReader

c70581d

adding import statement that was needed and fixing codestyle

643a6e2

ssktotoro requested review from bagxi, ditwoo and Scitator as code owners March 31, 2021 04:21

Kevin Wang added 3 commits March 31, 2021 00:23

updated CHANGELOG

46eee49

removing unnecesary documentation

d126e5c

fixing codestyle

596d115

Kevin Wang added 2 commits March 31, 2021 13:02

updated docs and requirements

115eade

adding a space

3260296

Scitator reviewed Apr 1, 2021

View reviewed changes

Kevin Wang added 3 commits April 1, 2021 11:27

not importing nibabel for docs

ba10888

fixing typo

39d2b0c

adding step for contrib pip caching

323ff15

Scitator previously requested changes Apr 1, 2021

View reviewed changes

Scitator reviewed Apr 1, 2021

View reviewed changes

.github/workflows/codestyle.yml Outdated Show resolved Hide resolved

.github/workflows/codestyle.yml Outdated Show resolved Hide resolved

fixed codestyle and added a test for reader that uncovered a mistake :)

801aa3a

hopefully fixing imports

49a1291

Scitator reviewed Apr 1, 2021

View reviewed changes

Kevin Wang added 3 commits April 1, 2021 15:14

adding sergey suggested fix

129cd1d

fixing deploy_push.yml

27bdd8d

updating workflows

6afe4b1

Scitator merged commit 0722b1a into catalyst-team:master Apr 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Niftireader #1151

Niftireader #1151

ssktotoro commented Mar 31, 2021 •

edited

Scitator commented Mar 31, 2021

ssktotoro commented Mar 31, 2021

Scitator commented Mar 31, 2021

ssktotoro commented Mar 31, 2021

Scitator Apr 1, 2021 •

edited

Scitator commented Apr 1, 2021

Scitator Apr 1, 2021

ssktotoro Apr 1, 2021

Scitator Apr 1, 2021

ssktotoro Apr 1, 2021

Scitator Apr 1, 2021

Scitator left a comment

ssktotoro commented Apr 1, 2021

Scitator left a comment

Scitator Apr 1, 2021

ssktotoro commented Apr 1, 2021

Scitator commented Apr 2, 2021

		@@ -0,0 +1,39 @@
		from typing import Optional

		import nibabel as nib

	${{ runner.os }}-${{ matrix.python-version }}-pip-
	${{ runner.os }}-${{ matrix.python-version }}-pip-contrib

Niftireader #1151

Niftireader #1151

Conversation

ssktotoro commented Mar 31, 2021 • edited

Before submitting (checklist)

Description

Related Issue

Type of Change

PR review

Scitator commented Mar 31, 2021

ssktotoro commented Mar 31, 2021

Scitator commented Mar 31, 2021

ssktotoro commented Mar 31, 2021

Scitator Apr 1, 2021 • edited

Choose a reason for hiding this comment

Scitator commented Apr 1, 2021

Scitator Apr 1, 2021

Choose a reason for hiding this comment

ssktotoro Apr 1, 2021

Choose a reason for hiding this comment

Scitator Apr 1, 2021

Choose a reason for hiding this comment

ssktotoro Apr 1, 2021

Choose a reason for hiding this comment

Scitator Apr 1, 2021

Choose a reason for hiding this comment

Scitator left a comment

Choose a reason for hiding this comment

ssktotoro commented Apr 1, 2021

Scitator left a comment

Choose a reason for hiding this comment

Scitator Apr 1, 2021

Choose a reason for hiding this comment

ssktotoro commented Apr 1, 2021

Scitator commented Apr 2, 2021

ssktotoro commented Mar 31, 2021 •

edited

Scitator Apr 1, 2021 •

edited