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 save_history bug #78

Closed
wants to merge 1 commit into from
Closed

Conversation

me-pic
Copy link
Contributor

@me-pic me-pic commented Aug 22, 2024

Closes #76
Not sure if it's the most elegant way to do it, but it seems like it's fixing the issue..

Proposed Changes

  • Add a line to convert the list of np.int64 to np.array to convert it back to list using tolist(). This way we are making sure that the items in the list are type int and not np.int64

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!

@me-pic
Copy link
Contributor Author

me-pic commented Aug 22, 2024

@smoia There is a test that is failing in test_editor.py because it calls on_remove on a PhysioEditor object, but this attribute does not exist anymore. Should I also adress that in this PR ?

@smoia
Copy link
Member

smoia commented Aug 23, 2024

@me-pic can you bump the test failure message?

And no, I wouldn't address it here (also because see #78). I would open a different PR.
As maybe opening an extra PR to check that we have all tests configured to run in CircleCI - that could go together as setting up tests and making them not fail!

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.

save_history fails with TypeError
2 participants