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.

Sign up for GitHub

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

Jump to bottom

Create Annotation and Annotator classes #618

Closed
wants to merge 5 commits into from
Closed

Create Annotation and Annotator classes #618

wants to merge 5 commits into from

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Dec 13, 2021

Closes #617, but far from ready for review. Some of this PR will overlap with #607.

Changes proposed in this pull request:

  • Replace Dataset.annotations DataFrame with a dictionary of name-Annotation pairs.
  • Create Annotation class for storing annotations.
  • Create Annotator class for creating Annotations from Datasets.

@tsalo tsalo added annotate Issues related to the annotate module breaking-change PRs that change results or interfaces. neurostore Issues and PRs related to NeuroStore compatibility. refactoring Requesting changes to the code which do not impact behavior labels Dec 13, 2021
@codecov
Copy link

codecov bot commented Dec 13, 2021 •
edited
Loading

Codecov Report

Merging #618 (a11d7ec) into main (16f06b0) will decrease coverage by 0.30%.
The diff coverage is 50.00%.

❗ Current head a11d7ec differs from pull request most recent head 96814f5. Consider uploading reports for the commit 96814f5 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #618      +/-   ##
==========================================
- Coverage   85.76%   85.46%   -0.31%     
==========================================
  Files          40       40              
  Lines        4390     4423      +33     
==========================================
+ Hits         3765     3780      +15     
- Misses        625      643      +18
Impacted Files Coverage Δ
nimare/dataset.py 89.12% <ø> (ø)
nimare/base.py 82.60% <50.00%> (-5.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16f06b0...96814f5. Read the comment docs.

@tsalo
Copy link
Member Author

tsalo commented Dec 14, 2021 •
edited
Loading

I need some way to validate Annotations housed in the Dataset.annotations attribute when they're added to the attribute with the dictionary's __setitem__ method. Validation should probably include checking the IDs in the Annotation.label_weights DataFrame.

The problem is that neither the Annotation and any custom dictionary-like class we create will have access to the Dataset, so it would be the Dataset's responsibility to run the validation any time the dictionary's __setitem__ method is called... and how would it even know that had happened? I don't think properties have any way of monitoring that kind of thing, short of re-setting the whole attribute each time we want to set an item in it (i.e., treat the mutable dictionary-like object like it's immutable).

@tsalo
More work on Annotation class.
47c339b 
We need merge and slice methods to support merge and slice methods in Dataset. Still not sure how to validate Annotations w.r.t. Datasets as they're added to the Dataset's annotations attribute.
@tsalo tsalo changed the title [REF] Create Annotation and Annotator classes Create Annotation and Annotator classes Dec 14, 2021
tsalo added a commit to tsalo/NiMARE that referenced this pull request Dec 16, 2021
@tsalo
Remove Annotator class.
075a5f0 
The Annotator and Annotation classes will be developed in neurostuff#618.
@tsalo
Use fit method instead of transform.
96814f5 
This is because fit takes a Dataset and returns a different type of object. Transform takes a Dataset and returns a modified one.
@tsalo tsalo added this to the 0.1.0 milestone Dec 19, 2021
tsalo added a commit that referenced this pull request Jan 6, 2022
@tsalo
Use scikit-learn for LDAModel (#607)
7e094f2 
* Drop LDA.

* Delete 03_lda.py

* Use resources instead of test data.

* Bundle sklearn model in new class.

* More updates.

* Fix.

* Add test.

* Update 03_plot_lda.py

* Improve things.

* Link to CBMA documentation.

* Update 03_plot_lda.py

* Update api.rst

* More cleanup.

* Remove Annotator class.

The Annotator and Annotation classes will be developed in #618.

* Update 03_plot_lda.py

* Remove undefined base class.
@tsalo
Copy link
Member Author

tsalo commented Jun 13, 2022

Closing this. We can circle back to it after the NIMADS PR is merged.

@tsalo tsalo closed this Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
annotate Issues related to the annotate module breaking-change PRs that change results or interfaces. neurostore Issues and PRs related to NeuroStore compatibility. refactoring Requesting changes to the code which do not impact behavior
Projects
None yet
Milestone
0.1.0
Development

Successfully merging this pull request may close these issues.

Refactor storage of annotations in Datasets
1 participant
@tsalo