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

remove add_revenue_split() #39

Closed
cjyetman opened this issue Apr 28, 2024 · 0 comments · Fixed by #40
Closed

remove add_revenue_split() #39

cjyetman opened this issue Apr 28, 2024 · 0 comments · Fixed by #40

Comments

@cjyetman
Copy link
Member

add_revenue_split() is no longer used anywhere:
https://github.com/search?q=org%3ARMI-PACTA+add_revenue_split+NOT+is%3Aarchived+&type=code

Its use in workflow.transition.monitor was removed in RMI-PACTA/workflow.transition.monitor#142

and some unexpected behavior of it was re-implemented manually in web_tool_script_1.R here:
https://github.com/RMI-PACTA/workflow.transition.monitor/blob/5fbe241de91accdb47295ef7032ae3e97c0c519f/web_tool_script_1.R#L97-L106

# FIXME: this is necessary because pacta.portfolio.allocate::add_revenue_split()
#  was removed in #142, but later we realized that it had a sort of hidden
#  behavior where if there is no revenue data it maps the security_mapped_sector
#  column of the portfolio data to financial_sector, which is necessary later
portfolio <-
  portfolio %>%
  mutate(
    has_revenue_data = FALSE,
    financial_sector = .data$security_mapped_sector
  )

in RMI-PACTA/workflow.transition.monitor#149

@cjyetman cjyetman added ADO and removed ADO labels Apr 28, 2024
cjyetman added a commit that referenced this issue Apr 29, 2024
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 a pull request may close this issue.

1 participant