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

[FIX] Figures of components with index 0 get rendered now #793

Merged
merged 6 commits into from
Sep 14, 2021

Conversation

eurunuela
Copy link
Collaborator

@eurunuela eurunuela commented Sep 8, 2021

Closes #766 .

Changes proposed in this pull request:

  • Add console log for debugging purposes.
  • Accept components with index 0 for updating figures on reports.
  • Limit Bokeh version to <2.3.0.

@eurunuela eurunuela added the reports issues related to boilerplate generation or visual reports label Sep 8, 2021
@eurunuela eurunuela self-assigned this Sep 8, 2021
@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #793 (c4bebb1) into main (bb57384) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##             main     #793   +/-   ##
=======================================
  Coverage   93.20%   93.20%           
=======================================
  Files          27       27           
  Lines        2208     2209    +1     
=======================================
+ Hits         2058     2059    +1     
  Misses        150      150           
Impacted Files Coverage Δ
tedana/reporting/dynamic_figures.py 100.00% <100.00%> (ø)
tedana/reporting/html_report.py 100.00% <100.00%> (ø)
tedana/io.py 93.36% <0.00%> (ø)
tedana/decay.py 97.24% <0.00%> (ø)
tedana/stats.py 96.62% <0.00%> (ø)
tedana/utils.py 97.38% <0.00%> (ø)
tedana/combine.py 91.42% <0.00%> (ø)
tedana/__init__.py 100.00% <0.00%> (ø)
tedana/gscontrol.py 100.00% <0.00%> (ø)
tedana/selection/_utils.py 100.00% <0.00%> (ø)
... and 14 more

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 bb57384...fbbe0e1. Read the comment docs.

@tsalo
Copy link
Member

tsalo commented Sep 10, 2021

To be honest the diff from the auto formatting is making it hard to see what the meaningful changes were. Could you add in a couple of GitHub comments to point to the affected code?

Copy link
Collaborator Author

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

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

After sharing a couple of emails with angharaddecates from Neurostars we have found out that reports generated with Bokeh v2.3 or higher won't render at all. I've just added this constraint to info.py in c4bebb1 .


tap_callback_jscode = """
// Accessing the selected component ID
var data = source_comp_table.data;
var selected_idx = source_comp_table.selected.indices;
if(selected_idx > 0) {
console.log('Selected idx is ' + selected_idx)
if(selected_idx >= 0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tsalo This is the main contribution for what I recall.

@eurunuela
Copy link
Collaborator Author

This PR is much easier to review now. Thank you @tsalo for merging #758 .

Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @eurunuela !

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

The changes look good to me, but can you add in the dependency change to the "Changes proposed" section of the summary comment before we merge?

@eurunuela
Copy link
Collaborator Author

The changes look good to me, but can you add in the dependency change to the "Changes proposed" section of the summary comment before we merge?

Done! I had completely forgotten about it, thank you @tsalo !

I'll merge once the tests pass.

@eurunuela
Copy link
Collaborator Author

eurunuela commented Sep 14, 2021

I don't know what's wrong with the 3-echo test... it keeps hanging up... Given that this PR does NOT add anything that could conflict with the test, I'll merge.

Edit: I actually cannot merge 🤣 I've re-sent the job to run the 3-echo test, hopefully it will work now.

@jbteves
Copy link
Collaborator

jbteves commented Sep 14, 2021

Hm, we just had the same thing happen with main for some reason...

@eurunuela
Copy link
Collaborator Author

eurunuela commented Sep 14, 2021

Can someone with admin permissions merge so we can release? We can fix it in a different PR.

@tsalo
Copy link
Member

tsalo commented Sep 14, 2021

Sure, I'll push it through. We should open an issue about the three-echo problem though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reports issues related to boilerplate generation or visual reports
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figures may not render in interactive reports
3 participants