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

Issue879 bug visualization #893

Merged
merged 5 commits into from
Sep 11, 2023
Merged

Conversation

jhmigueles
Copy link
Collaborator

@jhmigueles jhmigueles commented Sep 4, 2023

Fixes #879 => the error was originated in g.plot5 when creating the file summary report. In lines 117 and 119, making sure that the number of valid hours and the fraction of night available are numeric vectors resolves the issue (N valid hours was read as a character vector).

Bug reported in Google Group

Checklist before merging:

  • Existing tests still work (check by running the test suite, e.g. from RStudio).
  • Added tests (if you added functionality) or fixed existing test (if you fixed a bug).
  • Updated or expanded the documentation.
  • Updated release notes in inst/NEWS.Rd with a user-readable summary. Please, include references to relevant issues or PR discussions.
  • Added your name to the contributors lists in the DESCRIPTION and CITATION.cff files.

@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2023

Codecov Report

Merging #893 (36e24ef) into master (36dd97b) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head 36e24ef differs from pull request most recent head 428dce8. Consider uploading reports for the commit 428dce8 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #893      +/-   ##
==========================================
- Coverage   78.12%   78.03%   -0.09%     
==========================================
  Files          91       91              
  Lines       12094    12102       +8     
==========================================
- Hits         9448     9444       -4     
- Misses       2646     2658      +12     
Files Changed Coverage Δ
R/g.part2.R 63.55% <100.00%> (+1.41%) ⬆️
R/g.plot5.R 84.44% <100.00%> (-1.67%) ⬇️

... and 1 file with indirect coverage changes

@vincentvanhees
Copy link
Member

Looks good, but I see that g.plot5 converts a variety of columns to numeric, which feels inefficient to do at this point. FOr example, visual report is not run in parallel and doing it here contributes to processing time.

Would it be possible to save all of them to numeric right in part 2 and part 4, such that we do not have to deal with this at every point in GGIR when those columns are loaded?

@jhmigueles
Copy link
Collaborator Author

Would it be possible to save all of them to numeric right in part 2 and part 4, such that we do not have to deal with this at every point in GGIR when those columns are loaded?

Done. This was only needed in part 2 (part 4 output was being stored as numeric). Now part 2 output variables are stored as numeric when appropriate and I have removed "as.numeric" functions in g.plot5.

@vincentvanhees vincentvanhees merged commit a990e0f into master Sep 11, 2023
8 checks passed
@vincentvanhees vincentvanhees deleted the issue879_bugVisualization branch September 13, 2023 08:24
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.

possible bug in part 3 visualisation
3 participants