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

Fix fitting and optimization steps #75

Merged
merged 5 commits into from
Mar 25, 2024
Merged

Conversation

drodarie
Copy link
Collaborator

@drodarie drodarie commented Mar 7, 2024

This pull request fixes 3 different issues in the fitting and inhibitory optimization steps:

  • During the fitting step, the slices indexes provided by the user are supposed correct (global positions as defined by the AIBS). However, the annotation volumes and ISH marker datasets can have an offset. It is now taken into account when loading the marker datasets.
  • The fitting should be applied only on the group regions selected. The other regions used to be fitted according to the Rest group. These regions are now ignored during fitting but will obtain and estimate during the optimization step.
  • An unnecessary test was preventing leaf regions to have an uncertain first estimate from the fitting when their parent region is given for certain. This should not prevent the optimization as the children region will see their estimates corrected according to the parent region. This test was preventing a case scenario where the user set a non leaf region to a certain density value (with certainty) but not its children.

drodarie added 3 commits March 7, 2024 19:15
if a mother region count is given as certain, children regions can be given as uncertain.
This can happen when literature provides certain counts in mother region (e.g., Striatum is purely inhibitory).
@drodarie drodarie changed the title Offset slices indexes to take into account marker dataset offset. Fix fitting and optimization steps Mar 15, 2024
@drodarie
Copy link
Collaborator Author

For some reason, the tests for the mtype step are now failing: test_mtype_densities.py
However, I do not see how any of my changes impacted these tests.

@drodarie drodarie requested a review from mgeplf March 15, 2024 14:08
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@2b89cfd). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #75   +/-   ##
=======================================
  Coverage        ?   97.72%           
=======================================
  Files           ?       22           
  Lines           ?     1451           
  Branches        ?        0           
=======================================
  Hits            ?     1418           
  Misses          ?       33           
  Partials        ?        0           
Flag Coverage Δ
pytest 97.72% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mgeplf
Copy link
Collaborator

mgeplf commented Mar 25, 2024

However, I do not see how any of my changes impacted these tests.

I fixed that in #76

@drodarie drodarie requested a review from mgeplf March 25, 2024 14:39
@mgeplf
Copy link
Collaborator

mgeplf commented Mar 25, 2024

Code looks good, I have to assume it's correct from the scientific standpoint. Reportedly, this has been used for the latest atlas release in staging (https://bbpteam.epfl.ch/project/issues/browse/BBPP134-1342?page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel&focusedCommentId=239469#comment-239469)

@mgeplf mgeplf merged commit 31a341d into main Mar 25, 2024
5 checks passed
@mgeplf mgeplf deleted the fix/fitting_slice_indexes branch March 25, 2024 15:08
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.

3 participants