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

[FIX] Adds f_max to right place #712

Merged
merged 2 commits into from
Apr 12, 2021
Merged

[FIX] Adds f_max to right place #712

merged 2 commits into from
Apr 12, 2021

Conversation

jbteves
Copy link
Collaborator

@jbteves jbteves commented Apr 8, 2021

Closes #710 .

Changes proposed in this pull request:

  • Threshold F-maps before making derivative metrics, rather than just before kappa/rho calculation

@jbteves jbteves changed the title Adds f_max to right place [FIX] Adds f_max to right place Apr 8, 2021
@jbteves jbteves requested review from handwerkerd and tsalo April 8, 2021 17:40
@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #712 (94f2a0e) into main (009ccc5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #712   +/-   ##
=======================================
  Coverage   93.64%   93.64%           
=======================================
  Files          26       26           
  Lines        2029     2029           
=======================================
  Hits         1900     1900           
  Misses        129      129
Impacted Files Coverage Δ
tedana/metrics/kundu_fit.py 95.58% <100.00%> (ø)

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 009ccc5...94f2a0e. Read the comment docs.

@handwerkerd handwerkerd added breaking change WIll make a non-trivial change to outputs bug issues describing a bug or error found in the project labels Apr 9, 2021
handwerkerd
handwerkerd approved these changes Apr 9, 2021
View reviewed changes
Copy link
Member

@handwerkerd handwerkerd left a comment

Choose a reason for hiding this comment

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

The html reports page is blank, as was fixed in #703. On my computer, this isn't happening when I create the report with main but is with this PR. I have no clue why there would be this difference since these two branches are nearly identical. I assume this will get fixed with merging, but, just in case, I wanted to mention anyway.

@jbteves
Copy link
Collaborator Author

jbteves commented Apr 12, 2021

I ran this branch on a few datasets and in all cases it correctly generated reports for me on my local machine. If there's something amiss we can open an issue to address it.

@jbteves jbteves merged commit 167231b into ME-ICA:main Apr 12, 2021
@jbteves jbteves deleted the fix_710 branch April 12, 2021 14:27
@handwerkerd handwerkerd mentioned this pull request Apr 12, 2021
Merged
jbteves added a commit that referenced this pull request May 4, 2021
@tsalo @jbteves
Use class for managing outputs (#711)
aa499b5 
* Initial work on a class.

* Clean up.

* More work.

* Revert mistake.

* [FIX] Adds f_max to right place (#712)

Moves f_max thresholding to the step where the R2/S0 maps are written out, rather than just before kappa/rho calculation.

* [FIX] Calculate Kappa and Rho on full F-statistic maps (#714)

* Calculate rho and kappa on full maps.

* Add missing test outputs.

* Revert changes to outputs.

* Fill out docstrings for file-writing class.

* More work.

* [MAINT] Drop 3.5 support and begin 3.8 and 3.9 support (#721)

* Drop 3.5 support and add 3.8/3.9 support.

* Drop 3.5 tests and add 3.8/3.9 tests.

* Update installation instructions.

* Progress on refactor

* Fix some errors

* Fix style

* Switch to fstrings where possible

* Update tedana/io.py

Co-authored-by: Taylor Salo <tsalo006@fiu.edu>

* Update tedana/io.py

Co-authored-by: Taylor Salo <tsalo006@fiu.edu>

* Address @handwerkerd docstring review

* Replace generator with io_generator

Co-authored-by: Joshua Teves <jbtevespro@gmail.com>
Co-authored-by: Joshua Teves <joshua.teves@nih.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsalo tsalo tsalo approved these changes

@handwerkerd handwerkerd handwerkerd approved these changes

Assignees
No one assigned
Labels
breaking change WIll make a non-trivial change to outputs bug issues describing a bug or error found in the project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

F maps are truncated after they are written but before kappa/rho calculation
3 participants
@jbteves @handwerkerd @tsalo