Standardize Header Handling when creating new nibabel object #758
Conversation
|
all changed except for ivadomed/ivadomed/loader/adaptative.py Line 753 in fb3c913 |
Pull Request Test Coverage Report for Build 1427913172
💛 - Coveralls |
cakester
changed the title
|
Going to need a bit more time to review this. Converting to draft for now. |
|
I think you just missed a few of the places when the NifTI images are instantiated, the header didn't get copied over. I think once you address those, we can sit with @charleygros again to quickly ensure these changes makes sense. I do not see any major issues in terms of implementation as we are replicating what was done in Inference.py without changing anything significantly. |
|
please see spinalcordtoolbox/spinalcordtoolbox#3295 (comment). I don't think it is the best strategy to implement the suggestion of #715 (comment) without strategizing on the potential consequences (e.g. nipy/nibabel#1012). |
|
@jcohenadad acknowledged. Just saw the latest update on the latest related issue and the closed upstream ticket. Sorry been missing context too long 😨. Will prioritize attempting the proposed resolution. Current action plan:
|
dyt811
added
priority:low
priority:medium
and removed
priority:medium
priority:low
labels
dyt811
changed the title
| @pytest.mark.script_launch_mode('subprocess') | ||
| def test_cascaded_architecture_tutorial(script_runner): | ||
| download_dataset("data_example_spinegeneric") | ||
| file_config = os.path.join(__data_testing_dir__, 'cascaded_architecture_tutorial_config.json') |
There was a problem hiding this comment.
note for this to work you have to upload the config.json file to the data_functional_test directory just like automated_test_config
| download_dataset("data_example_spinegeneric") | ||
| file_config = os.path.join(__data_testing_dir__, 'cascaded_architecture_tutorial_config.json') | ||
|
|
||
| ret = script_runner.run('ivadomed', '-c', f'{file_config}', |
There was a problem hiding this comment.
because of this function which gets executed in ivadomed main
`
def __get_commit(path_to_git_folder=None):
"""Get GIT ivadomed commit.
Args:
path_to_git_folder (str): Path to GIT folder.
Returns:
str: git commit ID, with trailing '*' if modified.
"""
if path_to_git_folder is None:
path_to_git_folder = __ivadomed_dir__
else:
path_to_git_folder = os.path.abspath(os.path.expanduser(path_to_git_folder))
p = subprocess.Popen(["git", "rev-parse", "HEAD"], stdout=subprocess.PIPE, stderr=subprocess.PIPE,
cwd=path_to_git_folder)
`
since the existance of os.path.abspath it assumes that current working directory is ivadomed root (which is not unless you run pytest inside the root directory) we can only execute this test inside ivadomed root directory or else it would flail...actually no it's the cwd argument for subprocess that's failing
There was a problem hiding this comment.
since the existance of os.path.abspath it assumes that current working directory is ivadomed root (which is not unless you run pytest inside the root directory) we can only execute this test inside ivadomed root directory or else it would flail
good catch @cakester ! it would be great to solve that issue.
There was a problem hiding this comment.
also, places such as
def write_derivatives_dataset_description(self, path_data):
"""Writes default dataset_description.json file if not found in path_data/derivatives folder
"""
filename = 'dataset_description'
deriv_desc_file = '{}/derivatives/{}.json'.format(path_data, filename)
label_desc_file = '{}/derivatives/labels/{}.json'.format(path_data, filename)
# need to write default dataset_description.json file if not found
if not os.path.isfile(deriv_desc_file) and not os.path.isfile(label_desc_file):
f = open(deriv_desc_file, 'w')
f.write('{"Name": "Example dataset", "BIDSVersion": "1.0.2", "PipelineDescription": {"Name": "Example pipeline"}}')
f.close()
assumes that path data is relative to the current running directory, which is not when we execute tests in the testing directory, that further prevents this test to be run inside testing dir
There was a problem hiding this comment.
problem solved, we change directory before running the tests!
There was a problem hiding this comment.
problem solved, we change directory before running the tests!
is this directory change now implemented? is it done automatically in the code?
There was a problem hiding this comment.
it's only done with this test, do you want it for all of ivadomed?
|
someone approve this ivadomed/data_functional_testing#6 |
testing/functional_tests/test_cascaded_architecture_tutorial.py, around 23 min for this test... |
That is actually fine. As tutorial typically run very long. We will not be incorporating this as part of the regular unit testing but an optional integration testing that is run on either a weekly or more optinally triggered. |
I have a suggestion for this. In the |
|
note once #865 this got in, we don't need cascade tutorial test here anymore |
dyt811
added this to In Progress
in Optional GitHub Action CI to able to test Tutorials End to End
|
This change has been empirically validated to address bugs surfaced in Tutorial 2 and prevent its crash as documented in the tutorial. This needs to go in before the tutorial testing related changes. |
|
This PR's change has been empirically validated in |
Checklist
GitHub
PR contents
Description
In PR #714 the way nibabel objects are created was changed, so we want to update the object creation in the following places as well
Linked issues
Timeline of the related issues:
Resolve #715
Resolve #775