-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Postprocessing frame check bug fix #431
Conversation
… more utitlity functions
Codecov Report
@@ Coverage Diff @@
## master #431 +/- ##
=======================================
Coverage 96.19% 96.19%
=======================================
Files 36 36
Lines 1604 1604
=======================================
Hits 1543 1543
Misses 61 61
Continue to review full report at Codecov.
|
I don't know how the previous code could result in a failure. Probably missing something here. Aren't the frames always from 1-... ? Isn't gif rendering would fail otherwise? Maybe add a test which would fail in the current version and is fixed by this. Btw make sure to update your personal fork before creating a PR. (For the next one don't worry about this one) as currently this PR has >30 commits but only the last is the one which isn't merged already |
Sorry for merging I though I did. The error was this, I think for javis is not an issue having for example frame 0 defined but with the old solution this would throw this error instead of using frame 0 normally, now it shouldn't anymore. I think it throws a warning but does not fail with frame 0 at least. Anyway even if it would fail anyway it is still better to have the regular error instead of this one that would be unexplicative. I will check again and add the test later today! |
So I adapted a test in postprocessing to account for the negative frames and it does work with this PR while it does not without it. |
Great thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks LGTM. Please add a line to the changelog :)
Then I'll merge and tag a new release
That should be it! |
PR Checklist
If you are contributing to
Javis.jl
, please make sure you are able to check off each item on this list:CHANGELOG.md
with whatever changes/features I added with this PR?Project.toml
+ set an upper bound of the dependency (if applicable)?test
directory (if applicable)?Link to relevant issue(s)
Closes #
How did you address these issues with this PR? What methods did you use?
The check for frames returned by
postprocess_frames_flow
was wrongly written, this should fix it.