Skip to content

Conversation

@aleks-sch
Copy link
Contributor

@aleks-sch aleks-sch commented Sep 8, 2023

cc @remi-schroders @grace-schroders

requirement to support 'target IDs' to enable traceability of which target ID powers temperature scores. is related to email trail screenshotted at end of this comment

  • new IDataProviderTarget optional field target_ids
  • logic to preserve target_ids as part of target_validation and temperature_score
  • test case EndToEndTest::test_target_ids to demonstrate target_ids being persisted through model for each case
  • merge add try/except to CTA-file request.get request for company firewall #308 before this PR (could not develop locally without that change)

image

still TODO

  • bump version number in setup.py

image

@aleks-sch
Copy link
Contributor Author

hi @mountainrambler can you plz take a look at this PR? Do you see any major issues? the new field is optional and should not degrade existing temperature score methodology

Copy link
Contributor

Choose a reason for hiding this comment

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

already squashed and merged

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do note reference a specific data provider. Make it generic. There are other data providers that also wish to see a target id.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, and thanks for the PEP8 updates and spelling corrections

@mountainrambler
Copy link
Contributor

I've tested this PR and it works well. Special thanks for the cleaning up of comments and PEP8 related fixes. I did comment on the reference to a specific data provider in interfaces.py. Please update and make it generic, since there are other data providers that are interested in introduction a target ID.

Also, we need to update the docs in conjunction with this release. I propose adding a column at the end of the target_data tab in the data provider excel file and the corresponding info in the docs page on Data Legends. Are there any other things we need to fix in relation to target IDs?

@mountainrambler
Copy link
Contributor

I suggest we bump this version to 1.1.0 since we are doing more than bug fixes. Then we can do the EOL on Python 3.7 afterwards and bump to 2.0.0. Thoughts?

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.

2 participants