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

move models to own files/simplify view #123

Merged
merged 6 commits into from
Dec 20, 2023

Conversation

alessandrofelder
Copy link
Member

@alessandrofelder alessandrofelder commented Dec 18, 2023

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

It is a step on the way towards being able to download new atlases, and update local atlases to newer versions, through a napari GUI.

What does this PR do?
This PR does two things (I should have split this into two PRs - sorry!) in preparation for moving atlas download functionality to a different widget/adding atlas update functionality in the GUI.

Make the model/views more flexible to allow several (kinds of) views on the same model

  • separates the datamodels for atlases and atlas regions ("structures") into separate files (they were in the view file before).
  • Renames AtlasTableView to AtlasViewerView, which is the view that allows users to "view" atlases in napari. This paves the way to have other atlas views, e.g. an AtlasUpdaterView or so, which is a view that allows updating the atlas version, using the same data model.
  • Removes functionality to download atlases in the current BrainrenderWidget, as this will be for visualisation purposes only.

Minor refactor of the atlas table model

This PR does a minor refactor of AtlasTableModel (see below for details).

  • allows the model to be updated via the atlas api(e.g. when new atlas has been downloaded) without needing re-instantiation
  • beautifies how the atlas name is displayed.

I've sign-posted these new functions with a comment.
The data model code has not changed otherwise (beyond moving files).

References

Will facilitate #21 by separating concerns further.

How has this PR been tested?

Existing tests refactored in line with changes detailed above, pass locally and on CI.

Is this a breaking change?

Yes, in the sense that downloading atlases will now not be possible via the viewer widget.

Does this PR require an update to the documentation?

Once #21 is addressed, yes, we'll need to update https://brainglobe.info/tutorials/visualise-atlas-napari.html

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (unit & integration)
  • [n/a] The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

in view of moving to a separate version managing widget
Copy link

codecov bot commented Dec 18, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (fa68926) 97.44% compared to head (3b5c7ea) 97.72%.

Files Patch % Lines
brainrender_napari/widgets/atlas_viewer_view.py 98.18% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #123      +/-   ##
==========================================
+ Coverage   97.44%   97.72%   +0.28%     
==========================================
  Files           8        9       +1     
  Lines         391      396       +5     
==========================================
+ Hits          381      387       +6     
+ Misses         10        9       -1     

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

Comment on lines +21 to +46
def refresh_data(self) -> None:
"""Refresh model data by calling atlas API"""
all_atlases = get_all_atlases_lastversions()
data = []
for name, latest_version in all_atlases.items():
if name in get_atlases_lastversions().keys():
data.append(
[
name,
self._format_name(name),
get_local_atlas_version(name),
latest_version,
]
)
else:
data.append(
[name, self._format_name(name), "n/a", latest_version]
)

self._data = data

def _format_name(self, name: str) -> str:
formatted_name = name.split("_")
formatted_name[0] = formatted_name[0].capitalize()
formatted_name[-1] = f"({formatted_name[-1].split('um')[0]} \u03BCm)"
return " ".join([formatted for formatted in formatted_name])
Copy link
Member Author

Choose a reason for hiding this comment

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

These are the new functions added to the atlas table model.

@alessandrofelder alessandrofelder marked this pull request as ready for review December 19, 2023 10:29
Copy link
Member

@IgorTatarnikov IgorTatarnikov left a comment

Choose a reason for hiding this comment

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

Looks good, just a few comments on my end!

Comment on lines 53 to 55
for hidden_column in [0, 2, 3]:
# hide raw name(0), local and latest versions(2,3) in this view
self.hideColumn(hidden_column)
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to avoid hard coding the hidden columns? I can foresee a situation where the column order is changed, or new columns are added throwing this off.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point - I can implement this with a simple refactor of our custom model.

brainrender_napari/widgets/atlas_viewer_view.py Outdated Show resolved Hide resolved
Comment on lines +25 to +38
for name, latest_version in all_atlases.items():
if name in get_atlases_lastversions().keys():
data.append(
[
name,
self._format_name(name),
get_local_atlas_version(name),
latest_version,
]
)
else:
data.append(
[name, self._format_name(name), "n/a", latest_version]
)
Copy link
Member

Choose a reason for hiding this comment

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

What does 'get_local_atlas_version' return if the atlas isn't downloaded? If it's something sensible can that be used instead of querying 'get_atlases_lastversions()'?

Suggested change
for name, latest_version in all_atlases.items():
if name in get_atlases_lastversions().keys():
data.append(
[
name,
self._format_name(name),
get_local_atlas_version(name),
latest_version,
]
)
else:
data.append(
[name, self._format_name(name), "n/a", latest_version]
)
for name, latest_version in all_atlases.items():
local_version = get_local_atlas_version(name) if get_local_atlas_version else "n/a"
data.append(
[
name,
self._format_name(name),
local_version,
latest_version,
]
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't think it currently returns something sensible unfortunately - I've tested locally and it gives a list index out of range error. I've opened brainglobe/brainglobe-atlasapi#187 as a possibility for the future.

tests/test_unit/test_atlas_table_model.py Outdated Show resolved Hide resolved
alessandrofelder and others added 3 commits December 20, 2023 17:02
Co-authored-by: Igor Tatarnikov <61896994+IgorTatarnikov@users.noreply.github.com>
Copy link
Member

@IgorTatarnikov IgorTatarnikov left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@IgorTatarnikov IgorTatarnikov merged commit ebf43dc into main Dec 20, 2023
12 checks passed
@IgorTatarnikov IgorTatarnikov deleted the separate-datamodels-and-views branch December 20, 2023 17:03
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.

None yet

2 participants