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

BlueprintInput deepcopies inputs at initialization #358

Merged
merged 12 commits into from
Nov 30, 2020

Conversation

vinferrer
Copy link
Collaborator

@vinferrer vinferrer commented Nov 26, 2020

Closes #314

As described in #314 ```check_trigger_amountsmethod inside theBlueprintInput`` class was modifying the attributes of the objects and at the same time modifying the inputs to the class initialization. Now at the initialization the inputs are deepcopied to avoid these problems

Proposed Changes

  • Deepcopy BlueprintInput inputs at initialization
  • Add assert to test this
  • Eliminate deepcopies from the testing

Change Type

  • bugfix (+0.0.1)
  • minor (+0.1.0)
  • major (+1.0.0)
  • refactoring (no version update)
  • test (no version update)
  • infrastructure (no version update)
  • documentation (no version update)
  • other

Checklist before review

  • I added everything I wanted to add to this PR.
  • [Code or tests only] I wrote/updated the necessary docstrings.
  • [Code or tests only] I ran and passed tests locally.
  • [Documentation only] I built the docs locally.
  • My contribution is harmonious with the rest of the code: I'm not introducing repetitions.
  • My code respects the adopted style, especially linting conventions.
  • The title of this PR is explanatory on its own, enough to be understood as part of a changelog.
  • I added or indicated the right labels.
  • I added information regarding the timeline of completion for this PR.
  • Please, comment on my PR while it's a draft and give me feedback on the development!

@vinferrer vinferrer marked this pull request as draft November 26, 2020 11:23
@vinferrer
Copy link
Collaborator Author

Ah, so it wasn't showing up in the tests before, my mistake

@eurunuela
Copy link
Collaborator

Ah, so it wasn't showing up in the tests before, my mistake

It's failing because you're comparing a variable called text_index that you haven't declared.

@tsalo
Copy link
Member

tsalo commented Nov 26, 2020

Sorry about that. I couldn't put a comment down where the assert should have gone, but I didn't think the variable hadn't been declared yet where I did comment.

@vinferrer
Copy link
Collaborator Author

wait so should it pass once the variable is declared?

@tsalo
Copy link
Member

tsalo commented Nov 26, 2020

If the original variable is stored in the object (i.e., the bug persists), then it will fail. If it isn't then it should pass.

@vinferrer
Copy link
Collaborator Author

okay I think this will do then

@eurunuela
Copy link
Collaborator

okay I think this will do then

Then make it ready for review 😉

@vinferrer vinferrer changed the title Do not use copy() in test_phyisio_obj.py Check_trigger_amounts deepcopies inputs Nov 27, 2020
@vinferrer vinferrer changed the title Check_trigger_amounts deepcopies inputs Blueprint_input deepcopies inputs at inicialization Nov 27, 2020
@vinferrer vinferrer changed the title Blueprint_input deepcopies inputs at inicialization BlueprintInput deepcopies inputs at inicialization Nov 27, 2020
@vinferrer vinferrer added the BugFIX This PR generally closes a `Bug` issue, and increments the patch version (0.0.+1) label Nov 27, 2020
@vinferrer vinferrer requested a review from eurunuela November 27, 2020 10:08
@vinferrer vinferrer marked this pull request as ready for review November 27, 2020 10:09
Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

Execution looks good to me!

However, there's style-related point to address: can you fix the library to pass style checks and adjust the order of the imports in physio_obj?

I'm going to prepare all the rest to be able to merge this in.

@eurunuela eurunuela changed the title BlueprintInput deepcopies inputs at inicialization BlueprintInput deepcopies inputs at initialization Nov 27, 2020
@smoia
Copy link
Member

smoia commented Nov 27, 2020

Now that I think about it: @vinferrer could you make the BlueprintOutput initialisation similarly creating deepcopies of the inputs please?

@vinferrer vinferrer requested a review from smoia November 27, 2020 12:01
@eurunuela
Copy link
Collaborator

Could you please fix the style? We already have a non-passing badge on the README, we should avoid a second one 😅

Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

Thank you @vinferrer !

My approval is dependent on passing all tests. Please fix the style issues.
(and possibly next time don't introduce them 😉)

@vinferrer
Copy link
Collaborator Author

vinferrer commented Nov 27, 2020

Thank you @vinferrer !

My approval is dependent on passing all tests. Please fix the style issues.
(and possibly next time don't introduce them 😉)

The changes required for the test to pass are in the PR #349, which is waiting for your review @smoia 😉

@smoia
Copy link
Member

smoia commented Nov 27, 2020

The changes required for the test to pass are in the PR #349, which is waiting for your review @smoia wink

Well, I don't know when we'll merge #349. So please fix them here so we don't propagate them any further.

@codecov
Copy link

codecov bot commented Nov 27, 2020

Codecov Report

Merging #358 (fb36274) into master (9e29b7a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #358   +/-   ##
=======================================
  Coverage   94.80%   94.80%           
=======================================
  Files           8        8           
  Lines         808      809    +1     
=======================================
+ Hits          766      767    +1     
  Misses         42       42           
Impacted Files Coverage Δ
phys2bids/physio_obj.py 92.89% <100.00%> (+0.03%) ⬆️

Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

LGTM!

@smoia
Copy link
Member

smoia commented Nov 27, 2020

(@tsalo if you're happy and you know it, merge this in! 👏 👏 )

@vinferrer vinferrer requested a review from tsalo November 30, 2020 07:55
@tsalo tsalo merged commit fa5238c into physiopy:master Nov 30, 2020
@smoia
Copy link
Member

smoia commented Nov 30, 2020

🚀 PR was released in 2.3.1 🚀

@smoia smoia added the released This issue/pull request has been released. label Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugFIX This PR generally closes a `Bug` issue, and increments the patch version (0.0.+1) released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BlueprintInput.check_trigger_amount() modifies inputs
4 participants