Skip to content

Adding Ara columns for Composition values#234

Merged
maxhutch merged 4 commits intodevelopfrom
feature/PLA-3096-composition-columns
Feb 26, 2020
Merged

Adding Ara columns for Composition values#234
maxhutch merged 4 commits intodevelopfrom
feature/PLA-3096-composition-columns

Conversation

@maxhutch
Copy link
Copy Markdown
Contributor

Citrine Python PR

Description

This PR adds four column definitions for composition-valued variables in Ara definitions.

PR Type:

  • Breaking change (fix or feature that would cause existing functionality to change)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Adherence to team decisions

  • I have added tests for 100% coverage
  • I have written Numpy-style docstrings for every method and class.
  • I have communicated the downstream consequences of the PR to others.

@maxhutch maxhutch marked this pull request as ready for review February 26, 2020 18:33
Comment on lines +271 to +272
n: int
index of the component name to extract, starting with 1 for the biggest
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What happens if n is larger than the number of components? For NthBiggestComponentQuantityColumn I would expect to get a value of 0, but I do not know what to expect here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Its empty. I'll clarify in docs.


def __init__(self, *,
data_source: str,
n: int):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we set n=1 as the default? I think it depends on how we expect people to use this column (will they almost always be grabbing the largest amount?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Given that "N" is in the name, I'd say "no".

@maxhutch maxhutch requested a review from bfolie February 26, 2020 19:17
@maxhutch maxhutch merged commit 4ff2ebd into develop Feb 26, 2020
@maxhutch maxhutch deleted the feature/PLA-3096-composition-columns branch February 26, 2020 19:20
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