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

Dbodor/prospector fixes #95

Open
wants to merge 8 commits into
base: dbodor/tdd_collapsed
Choose a base branch
from

Conversation

DaniBodor
Copy link
Contributor

@DaniBodor DaniBodor commented Jul 8, 2023

prospector found a bunch of linting issues, which I (mostly) resolved here.

A few remaining that I wasn't sure of are tagged in comments below

I accidentally branched this off another branch, so will point this PR to there for now.

cvasl/carve.py Outdated
@@ -438,7 +429,7 @@ def fetch_image(self, dicom_file):
# not a file name.
self.reader.SetFileName(dicom_file)
dcm = self.reader.Execute()
return sitk.GetArrayFromImage(image)
return sitk.GetArrayFromImage(dcm) #TODO check that dcm is correct parameter instead of `image`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume this is what you meant, correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not do anything on any function that deals with images directly, because we don't have them. You are probably right, but this worked on some version of an image I had. Please hold off until we have our actual images to repair carve.

cvasl/cli.py Outdated
@@ -110,7 +110,7 @@ def make_parser():
return parser


def main(argv):
def main(argv): #pylint: disable=unused-argument
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it required to pass an argument here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is how I am used to dealing with command-line argparse. I suppose there could be some clever way to write it without an argument,,, but I don't know it.

cvasl/mold.py Outdated
@@ -191,7 +194,7 @@ def debias_folder(file_directory, algorithm, processed, force=False):
if algorithm == 'n4_debias_sitk':
array = n4_debias_sitk(file)
elif algorithm == 'alternative_debias_a':
array = alternative_debias_a(file)
array = alternative_debias_a(file) #TODO: what is this?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function doesn't exist. Is it a future to do? In that case, shall we just raise a NotImplementedError for now and add a #TODO about it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, exactly @DaniBodor it's a function we didn't write. And since it deals with images, we should hold off until we are working on images.

@DaniBodor DaniBodor changed the base branch from main to dbodor/tdd_collapsed July 8, 2023 08:04
@DaniBodor DaniBodor changed the base branch from dbodor/tdd_collapsed to drcandacemakedamoore/tdd July 8, 2023 08:05
@DaniBodor DaniBodor changed the base branch from drcandacemakedamoore/tdd to dbodor/tdd_collapsed July 8, 2023 08:05
@drcandacemakedamoore
Copy link
Collaborator

You can also just run python setup.py lint (or if you have just done it python setup.py lint --fast). I will lint the other branch you made now, and the rest of these issues should wait until we are dealing with our specific images, so we implement the right thing.

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