Skip to content

Refactor summary report generation: improve formatting and update example paths - #33

Merged
AbhirupaGhosh merged 4 commits into
mainfrom
minor-update-summary
Aug 4, 2026
Merged

Refactor summary report generation: improve formatting and update example paths#33
AbhirupaGhosh merged 4 commits into
mainfrom
minor-update-summary

Conversation

@AbhirupaGhosh

Copy link
Copy Markdown
Contributor

No description provided.

@AbhirupaGhosh AbhirupaGhosh self-assigned this Jul 28, 2026

@jananiravi jananiravi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@AbhirupaGhosh the colors are likely inconsistent. please check!

Image Image

rest looks good.

Comment thread R/amRdataPlots.R
Comment thread R/amRdataPlots.R Outdated
@AbhirupaGhosh

Copy link
Copy Markdown
Contributor Author

@AbhirupaGhosh the colors are likely inconsistent. please check!

Image Image
rest looks good.

I changed the color to blue because we had a discussion to change the grey to blue during my poster preparation.

@jananiravi

Copy link
Copy Markdown
Member

@AbhirupaGhosh the colors are likely inconsistent. please check!

Image Image

rest looks good.

I changed the color to blue because we had a discussion to change the grey to blue during my poster preparation.

I like blue more but tandem changes need to immediately go into amRviz so we don't forget. cc: @eboyer221

Comment thread R/amRdataPlots.R
@eboyer221

Copy link
Copy Markdown
Contributor

@AbhirupaGhosh the colors are likely inconsistent. please check!

I like blue more but tandem changes need to immediately go into amRviz so we don't forget. cc: @eboyer221

Confirmed that the color change is already in sync between both amRviz and amRdata. I am pushing a small commit to fix a comment in PHENOTYPE_COLORS that currently reads "Susceptible is intentionally neutral grey" - changing to "muted blue" in the text.

@eboyer221

Copy link
Copy Markdown
Contributor

I pushed a few commits to help address the comments on this PR for the following issues:

  1. @examples erroring in check - I wrapped both generateSummary() and generatePlots() examples in \dontrun{}, since neither data/metadata.parquet nor results/metadata.parquet exist in the package. Verified with tools::Rd2ex() that they now render as ## Not run: and won't execute during R CMD check.
  2. cleanMetadata() → cleanMetaData() - the doc reference in generatePlots()'s description didn't match any real function. Corrected it to cleanMetaData(), which is what actually exists in R/data_processing.R and performs the drug_class/drug_abbr join.
  3. @return for generateSummary() - now documents that it invisibly returns md_path, matching how generatePlots() documents its return value.

I also reran devtools::document() to sync man/generateSummary.Rd and man/generatePlots.Rd with the updated roxygen comments (NAMESPACE unaffected - no export changes). Manually re-ran both functions end-to-end against data to confirm nothing broke.

eboyer221
eboyer221 previously approved these changes Aug 4, 2026

@eboyer221 eboyer221 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jananiravi All three review threads should be resolved now with the fixes I just pushed. Could you take another look when you get a chance to make sure the changes that were requested have now been completed? Thank you.

Comment thread R/amRdataPlots.R
@AbhirupaGhosh
AbhirupaGhosh merged commit e63259e into main Aug 4, 2026
@AbhirupaGhosh
AbhirupaGhosh deleted the minor-update-summary branch August 4, 2026 22:34
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.

3 participants