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

Chose an appropriate DATA_SHAPE #304

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@vessemer
Copy link
Contributor

vessemer commented Jan 30, 2018

Uncommented tests, as was mentioned in this comment.

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well

@vessemer vessemer referenced this pull request Jan 30, 2018

Merged

Classification model pipeline #298

1 of 1 task complete
@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Jan 30, 2018

Various memory errors. Restarted the build to see if that helps.

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Jan 30, 2018

@reubano, It happens time to time 'cause of unreasonably large choice of the DATA_SHAPE. The average maximum height of lungs for a grown man, according to this research (Table 2), is 282mm with std equals to 22mm and median 274mm (Table 2). Assuming normal distribution, 3 sigmas aside the mean will lead to the probability of case h belong to the interval P{|282 - h| <= 66} ~= 0.9974. Taking into an account the fact that all algorithms which were described (not only implemented ones) works with spacing on z-axis >= 0.9, therefore, having an upper bound value of (282 + 66) / 0.9 = 386 (voxels) for z-axis should be enough for all cases.

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Jan 30, 2018

I've made DATA_SHAPE to be (512, 512, 512) which should fit in memory while having reserved space for the z-axis.

@vessemer vessemer force-pushed the vessemer:298_classification_model branch from 2b6ef36 to e4ba764 Jan 30, 2018

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Jan 30, 2018

@reubano, done!

@lamby lamby changed the title All tests acitve All tests active Jan 31, 2018

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Jan 31, 2018

Can you clarify what you mean by "active" here? :)

@vessemer vessemer changed the title All tests active Chose an appropriate DATA_SHAPE Jan 31, 2018

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Jan 31, 2018

@lamby, initially I've created this PR to uncomment (activate) all tests, which I forget to uncomment in my previous PR #298, but then I've decided to figure out an appropriate DATA_SHAPE, along with other fixes, thus I've renamed this PR.

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Jan 31, 2018

Great work @vessemer! I think the main issue is the inclusion of "other fixes" unrelated to DATA_SHAPE things. Did you just happen to notice that some thing didn't look right (like the print statements)? Or were some tests still failing?

If the former, we will need to (at some point) add appropriate tests to make sure those fixes aren't mistakenly reverted in the future.

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Jan 31, 2018

Thanks, @reubano! Let me describe in a bit more details: commit All tests acitve is just to uncomment the commented tests, then Codestyle of evaluate_detection.py commit is merely about that some thing didn't look right, though in the last commit I've included (additional to the DATA_SHAPE things):

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Jan 31, 2018

Codestyle of evaluate_detection.py commit is merely about that some thing didn't look right,

Does this mean it wasn't previously being caught by the code linter that travis runs?

Remove redundant self.scale_factor which is neither dependend on DATA_SHAPE nor on self.input_shape by this and this.

This just sounds like a refactoring, so the current test set should (hopefully) be able to confirm nothing broke

calculate_volume function now accept centroids in real world units (mm) if meta was provided, which is logical and also leads to renewal of the tests

This one looks like an enhancement. I noticed that the x and z values were swapped. Is that something specific to using mm units? Or was it a latent bug that you caught?

--

Again, overall great work! And I think we may need to split this up into at least 2 individual PRs. One addressing the memory error (DATA_SHAPE) and commented out tests.

This 1st PR could also address the codestyle as long as the current linters properly identified the problem. If not, then a 2nd PR would fix the codestyle and the linter. This would all be lumped together with a "get tests passing" type of title.

If the x and z values swapping is specifically related to mm units, it can just be included in the last PR to address the mm units and the related test adjustments. Otherwise, it should probably go in a sep PR as a bug fix (with the appropriate test that shows it was indeed a bug).

Does that make sense?

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Jan 31, 2018

Does this mean it wasn't previously being caught by the code linter that travis runs?

Not all of them, 'cause code linter indicate E999 SyntaxError: invalid syntax and not proceed after that.

This just sounds like a refactoring, so the current test set should (hopefully) be able to confirm nothing broke

That's true.

This one looks like an enhancement. I noticed that the x and z values were swapped. Is that something specific to using mm units? Or was it a latent bug that you caught?

The swap is done due to the xyz way to store volumetric data pushed by the PR #44. It didn't matter for the artificial examples until mm units case were included in the tests, therefore it's related to the last commit.

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Jan 31, 2018

Just saw your new PRs. Let's see how travis likes them :).

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Jan 31, 2018

@reubano reubano referenced this pull request Feb 1, 2018

Merged

Fix memory and styling errors #307

1 of 1 task complete
@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Feb 1, 2018

Superceded by PRs #307 and #306

@reubano reubano closed this Feb 1, 2018

@vessemer vessemer deleted the vessemer:298_classification_model branch Feb 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment