Skip to content

Conversation

@cjyetman
Copy link
Member

@cjyetman cjyetman commented Jan 23, 2025

In the current template's report text, the percentages of covered assets by value for Equity and Bonds should match the percentage values in the sector coverage by value pie charts. However, it is currently using total_portfolio_percentage_equity and total_portfolio_percentage_equity which is the percent of Equity and Bonds within the total portfolio value.

Since the desired values are not exported by calculate_report_content_variables(), this PR adds those values to be exported. Additionally, it utilizes those values in the text of the integrated template. For templates typically used on Transition Monitor, those templates will need to be similarly updated in https://github.com/RMI-PACTA/templates.transition.monitor

The original change in the template text to use these newly desired values occurred in https://github.com/RMI-PACTA/templates.transition.monitor/pull/2 and then an incorrect fix to that occurred in https://github.com/RMI-PACTA/templates.transition.monitor/pull/18

@jdhoffa note that the percentages shown in the value pie charts are calculated within the JavaScript, therefore I had to calculate them separately to make it available for injection into the HTML text:

.text(prcnt_format(total_exploded_value / chart.total_value) + labels_plot.comment[0])

@cjyetman cjyetman requested a review from MonikaFu as a code owner January 23, 2025 05:04
@codecov
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 0% with 26 lines in your changes missing coverage. Please review.

Project coverage is 0.79%. Comparing base (24221e7) to head (aec75c8).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
R/calculate_report_content_variables.R 0.00% 24 Missing ⚠️
R/create_interactive_report.R 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main    #103      +/-   ##
========================================
- Coverage   0.80%   0.79%   -0.02%     
========================================
  Files         25      25              
  Lines       1619    1644      +25     
========================================
  Hits          13      13              
- Misses      1606    1631      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jdhoffa
Copy link
Member

jdhoffa commented Jan 23, 2025

@jdhoffa note that the percentages shown in the value pie charts are calculated within the JavaScript, therefore I had to calculate them separately to make it available for injection into the HTML text:

Noted and makes sense!

@jdhoffa
Copy link
Member

jdhoffa commented Jan 23, 2025

⚠️ In review, I noted an API change in the function calculate_report_content_variables.
This function is only used within create_interactive report, where the new argument is also used. I see no other invocations of this function across our repos, and so I don't anticipate any problems:
https://github.com/search?q=org%3ARMI-PACTA%20calculate_report_content_variables&type=code


pacta_sectors_percent_total_value_equity <-
audit_file %>%
dplyr::filter(.data$valid_input == TRUE) %>%
Copy link
Member

Choose a reason for hiding this comment

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

😂

Copy link
Member

@jdhoffa jdhoffa left a comment

Choose a reason for hiding this comment

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

Looks great!

@cjyetman cjyetman merged commit 664fd21 into main Jan 23, 2025
9 checks passed
@cjyetman cjyetman deleted the add-percent-pacta-sectors-content-values branch January 23, 2025 09:14
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