rmi-pacta / pacta.data.validation Goto Github PK

most obvious/important targets
https://github.com/RMI-PACTA/pacta.data.preparation
https://github.com/RMI-PACTA/workflow.data.preparation
https://github.com/RMI-PACTA/pacta.portfolio.analysis
https://github.com/RMI-PACTA/workflow.transition.monitor

And replace those functions with consolidated and centralized faking functions.

nit: maybe add a fixme/comment about eventually being able to replace this with functions from pacta.data.validation? I'd rather see these faker functions consolidated into one location rather than duplicated many places (possibly with slight variation)

Originally posted by @cjyetman in https://github.com/RMI-PACTA/pacta.data.preparation/pull/286#discussion_r1159527963

https://github.com/RMI-PACTA/pacta.data.preparation/blob/main/tests/testthat/helper-fake_data.R

AB#10853

@cjyetman This is more of an open question, do you think this package would be a good place for such a thing to exist?

I was thinking of generating fake input portfolio data (with key columns: isin, market_value, currency), that represent a series of common cases:

• No data
• Data, but no ISINs in PACTA sectors
• Only equities
• Only bonds
  etc.

There is a complicating factor, which is that the ISINs would need to point to real companies in the ABCD.

We could envision, in parallel, generating fake ABCD (and every other dataset) so that we can invent the ISINs and ensure stability.

But in any case, just curious if you think this repo would be an ok place to host this sort of thing?

Alterantively, we could just host a function that generates a suite of test portfolios.

Relates to: https://github.com/RMI-PACTA/archive.pacta.data.preparation/issues/216

• These ISINs represent private equity
• Private equity is not considered to be included in the analysis, so it is totally fine that we flag them as "Invalid ISIN", because for the purposes of CTM/ P4I they ARE invalid ISINs (even if overall they are not)
• The likelihood that a user is inputting private equity is relatively low BUT even if they do, and they notice that we say "invalid ISIN" I highly doubt they will be surprised

It would be unfortunate to lose validation entirely just because of these handful of ISINs though... I would vastly prefer that you:

• Update pacta.data.validation::validate_financial_data() to have the following behaviour:
• Pass if the ISIN is formatted as you currently expect (checksum and all that)
• Warn if an ISIN of this particular type ((A-Z){2}(numeric/ special char){10})
• Error only if the ISIN is totally bunk (any other format)

and then implement pacta.data.validation in workflow.data.preparation

That way we will at least get a record of the presence of these ISINs in the logs every time we run data prep.

Originally posted by @jdhoffa in RMI-PACTA/workflow.data.preparation#196 (comment)

applies to both:

• pacta.data.validation::validate_financial_data()
• pacta.data.validation::validate_abcd_flags_equity()

related:

• related RMI-PACTA/workflow.data.preparation#196
• related RMI-PACTA/workflow.data.preparation#185
• facilitates RMI-PACTA/workflow.data.preparation#222

AB#10854

This package has about 95% test coverage, and is currently used in production.

@cjyetman do you think this package is ready to bump to "stable", and if not, are there specific tasks or issues that you think are blocking it from becoming so?

AB#10914

In asset_valid_units, it expects "tCO2/pkm":

In assert_valid_value_range_for_sector_unit, it expects "gCO2/pkm":

cc: @jacobvjk

          I have been questioning whether there should be both `assert_valid_*()` and `is_valid_*()` style functions for each data type it knows, one that errors or returns TRUE invisibly and one that returns TRUE or FALSE. That would be a rather large "interface" change. There are also a few significant things missing.

Originally posted by @cjyetman in #73 (comment)

AB#10915