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

allow skipping normalization when compute_cell_density #70

Closed
wants to merge 1 commit into from

Conversation

mgeplf
Copy link
Collaborator

@mgeplf mgeplf commented Feb 28, 2024

No description provided.

Copy link
Contributor

@lecriste lecriste left a comment

Choose a reason for hiding this comment

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

@mgeplf
Copy link
Collaborator Author

mgeplf commented Feb 28, 2024

@lecriste and @Sebastien-PILUSO can you try this, and see if it solves the problem you were running into?

@drodarie
Copy link
Collaborator

I have a counter proposal for this pull request. The issue happened because the outside of the Nissl used is set to 0. Why not solving the issue in the function normalize_intensity in utils.py?
I can provide a simple implementation.

@Sebastien-PILUSO
Copy link

Sebastien-PILUSO commented Feb 28, 2024

@drodarie why would you like to still apply the normalize_intensity function when all the processings applied in it are already applied to the input volume?

@drodarie
Copy link
Collaborator

Then the function will not do anything and that's fine. This way, we don't have an extra user option which would make the pipeline even more difficult to use.

@drodarie
Copy link
Collaborator

See my proposal here

@mgeplf
Copy link
Collaborator Author

mgeplf commented Feb 28, 2024

See my proposal here

If that's what you want, that works for me. I very much prefer fewer options that modify the behavior (ie: I agree with what you on we don't have an extra user option...)

@Sebastien-PILUSO
Copy link

Ok let's try

@Sebastien-PILUSO
Copy link

I ran the code using @drodarie's branch and this gave the same cell density file than on the main branch. So this is okay for me.

@mgeplf
Copy link
Collaborator Author

mgeplf commented Mar 19, 2024

Done better in #74

@mgeplf mgeplf closed this Mar 19, 2024
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