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 Results objects to work with Estimators' fit() method #104

Merged
merged 6 commits into from
Jun 5, 2022

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Jun 3, 2022

Closes #52.

Changes proposed:

  • Overwrite Estimators' .dataset_ attribute with None when calling fit(). fit_dataset() will then replace that with the actual Dataset if it is called. Otherwise, .dataset_ will remain None.
    • This allows us to initialize Results objects, which assign an attribute (.dataset) from Estimator.dataset_.
  • Check if Result.dataset is None before using it. When it is None, raise a ValueError.
  • Add warnings to Result method docstring about whether fit is okay or if you need fit_dataset.
    • This way, it is clear when fit-generated Results' methods can be used and when you need a fit_dataset-generated Result.
  • Test the new behavior.

@tsalo tsalo added the bug Something isn't working label Jun 3, 2022
@tsalo tsalo requested a review from JulioAPeraza June 3, 2022 16:19
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.60%. Comparing base (541894e) to head (afb605d).
Report is 27 commits behind head on master.

Files Patch % Lines
pymare/estimators/estimators.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
+ Coverage   87.44%   87.60%   +0.16%     
==========================================
  Files          13       13              
  Lines         844      863      +19     
==========================================
+ Hits          738      756      +18     
- Misses        106      107       +1     

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

Copy link
Collaborator

@JulioAPeraza JulioAPeraza left a comment

Choose a reason for hiding this comment

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

I left one small comment. Everything else looks good to me!

pymare/tests/test_results.py Outdated Show resolved Hide resolved
@tsalo tsalo merged commit f425ff9 into neurostuff:master Jun 5, 2022
@tsalo tsalo deleted the make-failing-tests branch June 5, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Results objects break if user called fit() rather than fit_dataset()
3 participants