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

[ENH] Add diagonal reference line to kappa/rho plot #625

Merged
merged 1 commit into from
Dec 7, 2020

Conversation

notZaki
Copy link
Contributor

@notZaki notZaki commented Dec 6, 2020

Closes #582.

Changes proposed in this pull request:

  • Add a diagonal line (y = x) in kappa/rho scatter plot to make it easier to identify which of the two is greater for each component

Other notes:

  • Here's what the figure looks like the for the three test datasets
    diagonalref
  • The above scatter plots do not match the ones in the three-echo and five-echo reports from an older run
  • Perhaps a bit more troubling: I got different results by changing the python environment. Gif below shows the scatter plot from the three-echo test using Python 3.6 (pre-installed/configured on server) and 3.8 (conda environment). 🤷
    three-echo

@jbteves
Copy link
Collaborator

jbteves commented Dec 7, 2020

Hi @notZaki ! Thanks so much for opening this PR. Would you mind mentioning the differences in results as an issue, since you've already started looking into it? I'll follow up with you there!

@tsalo
Copy link
Member

tsalo commented Dec 7, 2020

This addition looks great @notZaki. Thank you.

While I think the change itself is straightforward (even knowing little about bokeh), I want to figure out why the tests did not run. It looks like your branch is up-to-date with our main branch, so the issue isn't that the CI configuration is out-of-date. I did notice that you had CircleCI set up on your fork, which I think can cause problems for CI on the main repository, but it looks like this new branch doesn't have any builds even there.

@jbteves do you have any ideas what could be happening?

@jbteves
Copy link
Collaborator

jbteves commented Dec 7, 2020

I'm as confused as you are @tsalo. This is basically the same behavior we see with the all contributors bot, but that shouldn't apply here because it's a code change and there's no [skip-ci] tag. Sometimes pushing an empty commit correctly triggers things (which is annoying, but we could try it, especially since we usually squash and merge anyway).

@tsalo
Copy link
Member

tsalo commented Dec 7, 2020

Thanks @jbteves. Tagging @ME-ICA/tedana-devs then. Hopefully someone on the team will have some insight.

EDIT: Sorry, or we could ask for an empty commit.

@jbteves
Copy link
Collaborator

jbteves commented Dec 7, 2020

Let's try the empty commit first, just because it's easy. Would you mind either pushing an empty commit or okaying one of us to do so @notZaki ?

@notZaki
Copy link
Contributor Author

notZaki commented Dec 7, 2020

I tried making an empty commit. Doesn't seem like it made a difference---except the docs got built this time.

If someone else wants to push an empty commit, then feel free to do so.

I'll add that I did fiddle with my circle-ci settings some time ago. Though I wouldn't expect my mangled circle-ci configuration to affect the PR.

@jbteves
Copy link
Collaborator

jbteves commented Dec 7, 2020

I feel like it shouldn't affect things, since you haven't modified the CircleCI config in your PR, and it should trigger the tedana CircleCI run rather than one from your repo. Bizarrely, CircleCI didn't start any jobs at all for this PR from our dashboard. Let me see if I can come up with anything that might be causing this.

@eurunuela
Copy link
Collaborator

It may be related to the CircleCI settings on your end @notZaki

It could be the case that it's running the tests on your fork and not on the ME-ICA repo. Could you please share a screenshot of your CircleCI settings?

@notZaki
Copy link
Contributor Author

notZaki commented Dec 7, 2020

@eurunuela Looks like you got it right---the test were trying to run on my fork so the configuration on my end was the issue.

I deactivated/reactivated the project on my circle-ci to reset it, and looks like it's working now.

@eurunuela
Copy link
Collaborator

I'm glad it works now.

From my experience, ideally, your CircleCI should not have the notZaki/tedana project (I think CircleCI uses the "follow"/"unfollow" terms). This way, it would only run tests on the PRs in ME-ICA/tedana and that would avoid any issue like the one we've just had. 😉

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.

Tests are now passing (thank you everyone for figuring it out and fixing it) and the resulting reports look good to me. Approving.

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. Amazing, thanks @notZaki !

@jbteves
Copy link
Collaborator

jbteves commented Dec 7, 2020

@all-contributors please add @notZaki for code.

@allcontributors
Copy link
Contributor

@jbteves

I've put up a pull request to add @notZaki! 🎉

@jbteves jbteves merged commit c121195 into ME-ICA:master Dec 7, 2020
@notZaki notZaki deleted the diagonalrefline branch December 28, 2020 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add y = x line to Kappa/Rho scatterplot
4 participants