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

refactor: use attributes #594

Draft
wants to merge 495 commits into
base: main
Choose a base branch
from
Draft

refactor: use attributes #594

wants to merge 495 commits into from

Conversation

xgui3783
Copy link
Member

@xgui3783 xgui3783 commented May 14, 2024

@xgui3783
Copy link
Member Author

xgui3783 commented May 15, 2024

attn @AhmetNSimsek

per our discussion, here is the WIP

left to do:

refactor todo list

  • implement {method}_iter()
  • atlas elements
    • space
    • parcellation
      • class
      • get_region()
    • region
      • class
      • find()
      • top_most flag
      • get_regional_map()
      • get_centroids an _get_spatialprops
    • parcellation map @AhmetNSimsek
      • labelled
      • statistical
  • update siibra/attr/meta/doi -> siibra/attr/desc/url see FZJ-INM1-BDA/siibra-configurations@9806628
    • update schema
    • update key url -> value
  • update siibra/attr/data/voi -> siibra/attr/data/image see FZJ-INM1-BDA/siibra-configurations@9806628
    • update schema
  • update regionspec see FZJ-INM1-BDA/siibra-configurations@9806628
    • fix property key name -> value
  • port over features
    • connectivity
    • bold
    • live queries
      • allen see https://github.com/FZJ-INM1-BDA/siibra-python/blob/e2c5284f6a1f85a70556b583944339e26d960386/siibra/retrieval_new/api_fetcher/allen.py and

        siibra-python/foo.py

        Lines 68 to 82 in e2c5284

        assert siibra.descriptions.modality.vocab.GENE_EXPRESSIONS == siibra.descriptions.modality.Modality(value="Gene Expressions")
        julichbrain_29_hoc1_lh_pmap = Image(format="nii", space_id="minds/core/referencespace/v1.0.0/dafcffc5-4826-4bf1-8ff6-46b8a31ff8e2",
        fetcher="https://neuroglancer.humanbrainproject.eu/precomputed/data-repo-ng-bot/20210616-julichbrain-v2.9.0-complete-mpm/PMs/Area-hOc1/4.2/Area-hOc1_l_N10_nlin2ICBM152asym2009c_4.2_publicP_026bcbe494dc4bfe702f2b1cc927a7c1.nii.gz")
        region = Region(attributes=[julichbrain_29_hoc1_lh_pmap])
        gene_maoa = Gene(value="MAOA")
        gene_tac1 = Gene(value="TAC1")
        query = QueryParam(attributes=[julichbrain_29_hoc1_lh_pmap,
        gene_maoa,
        gene_tac1,
        siibra.descriptions.modality.vocab.GENE_EXPRESSIONS ])
        gene_expression = list(feature_get(query, Feature))
        print(gene_expression)
      • big brain intensity profile see d22f454
      • brainglobe
      • document how to add additional external connectors see d22f454
    • reimplement autocomplete all modalities see 44a7194
    • sample feature fetching see 44a7194
  • move cache to root level module
  • attr
    • image dataitem (Image subclassing Location) @AhmetNSimsek
    • add more attr comparison
    • ptcloud (and pt) to image
  • setup & test siibra-schema repo see https://github.com/FZJ-INM1-BDA/siibra-schema
  • consider using standard json-ld schema, context etc
  • Retrieval
    • Volume Fetcher @AhmetNSimsek
      • NiftiFetcher
      • NeuroglancerFetcher
      • NeuroglancerMeshFetcher
      • GiftiFetcher
      • FreesurferAnnotFetcher
    • File fetcher
  • Locations @AhmetNSimsek
    • boundingbox
    • pointcloud
      • contour
    • point
    • plane3d
    • polyline
  • Map assignment
    • Point and PointCloud (etc) assignment to labelled and statistical maps
    • Image assignment to maps
  • Unit tests
  • e2e tests (For now, we are using foos under examples to test along with migrated/translated examples)
  • Examples
  • Notebooks to reproduce code on siibra paper
  • Polish docs (Need to discuss)
  • Docstrings

@AhmetNSimsek
Copy link
Collaborator

region.find is not deterministic and rather slow. I am noting down to investigate later.

@AhmetNSimsek
Copy link
Collaborator

AhmetNSimsek commented Jun 25, 2024

@xgui3783, I think we should remove the dataclass decorators from classes that inherit other a class that has a dataclass decorator. Because it creates unexpected behaviors such as some methods are overridden and very difficult to debug as it is an unexpected behavior.
Regarding being explicit: if a class A inherits a class B with dataclass decorators, the developer would know that A is also a dataclass. So, by repeating the 2 lines required to decorate class A as a dataclass, we break DRY, we face unexpected behavior, and we don't gain anything besides being explicit which is not really necessary in this context.

This changes if the class has extra members. Then, it is needed to use the decorator as the __init__ needs to be overridden. I suggest we remove all we can and leave only the necessary implementations. What do you think?

@AhmetNSimsek AhmetNSimsek marked this pull request as draft June 25, 2024 07:38
@@ -107,13 +116,33 @@ def populate_regions(
parent_region = _dict_id_to_region[parent_id]
region.parent = parent_region

def tiff_to_nii(tiff_bytes: bytes, affine: np.ndarray) -> str:
Copy link
Collaborator

@AhmetNSimsek AhmetNSimsek Jul 5, 2024

Choose a reason for hiding this comment

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

I think this should go to commons_new.maps or we create a tiff fetcher.
I prefer the second one (EDT: tho it is difficult to assume tiffs are standard. We could make tiff.brainglobe_tiff_fetcher)

Copy link
Member Author

Choose a reason for hiding this comment

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

as you alluded to, there is no standard tiff format.

That's why I think it should be here (i.e. brainglobe standard)


_registered_atlas.add(atlas_name)
logger.info(f"{atlas_name} added to siibra.")

return space, parcellation
return space, parcellation, labelled_map
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just food for discussion: since a labelled_map has to have this space and parcellation as a member, or computed property, perhaps we should just return the map.
The user can then access to parcellation and space either from mp.parcellation/space or from the registry

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind either way (I am also fine not returning anything)

The only reason why I return all three is to be explicit, to show all of the items that are added

@AhmetNSimsek
Copy link
Collaborator

AhmetNSimsek commented Jul 30, 2024

To profile and improve:

  • get_map is very slow. It does not speed up after first use. fixed @xgui3783
  • [ ]

Known bugs:

  • [ ]

Other issues:(

  • get_parcellation does not return the newest parcellation : cannot reproduce @xgui3783
  • region.get_boundingbox(space_spec) needs to be implemented

@xgui3783 Please edit and add/update more as we move forward to keep track of finer details

xgui3783 and others added 29 commits October 9, 2024 15:59
workflow: add debug
feat: attribute_collection now exposes categorization as a human readable str
feat: searchresult indexes categorizations when building summary table
fix: preprocess region
maint: low ram/non local as perf default
feat: append all dataop for volumes
feat: query cursor data table & reconfigure
doc: live queries base class
fix (e2e): use correct error class
feat: add siibra error class
refactor: (sparse)map.extract_full_map
fix: merge label nifti data operation
doc: zip fetcher description
test(e2e): fix bold, fix allen, fix image
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants