Skip to content

Pla 3725/rename inorganic chemical descriptor#288

Merged
bfolie merged 12 commits intomasterfrom
PLA-3725/rename-inorganic-chemical-descriptor
Apr 13, 2020
Merged

Pla 3725/rename inorganic chemical descriptor#288
bfolie merged 12 commits intomasterfrom
PLA-3725/rename-inorganic-chemical-descriptor

Conversation

@bfolie
Copy link
Copy Markdown

@bfolie bfolie commented Apr 8, 2020 •
edited by Imperssonator
Loading

Citrine Python PR

Description

Renames InorganicDescriptor to ChemicalFormulaDescriptor and removes the threshold argument from the constructor. The backend implementation still has a threshold parameter so we need to take it into account during serde, but it's less exposed to the user now.

If a user tries to create an InorganicDescriptor they will instead get a ChemicalFormulaDescriptor and they will see a deprecation warning.

Corresponding changes to nextgen-devkit (merged): https://github.com/CitrineInformatics/nextgen-devkit/pull/414/files#diff-b9e1cad38f41a2a025680df6f77d2aacR6-R9

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.
  • I have bumped the version in setup.py

@andyczerwonka
Copy link
Copy Markdown
Contributor

andyczerwonka commented Apr 9, 2020

We'll have to reach out DS so that they can update notebooks.

@bfolie
Copy link
Copy Markdown
Author

bfolie commented Apr 9, 2020

We'll have to reach out DS so that they can update notebooks.

To make the transition smoother I updated the PR to include a deprecated stub for InorganicDescriptor. This should mean that nobody's notebook will break, but they will get a warning message about deprecation.

@Imperssonator is included here because he's the primary user. We'll figure out a way to make sure everybody else knows as well.

@andyczerwonka
Copy link
Copy Markdown
Contributor

andyczerwonka commented Apr 9, 2020

To make the transition smoother I updated the PR to include a deprecated stub for InorganicDescriptor. This should mean that nobody's notebook will break, but they will get a warning message about deprecation.

I saw that, which is awesome. That’s how I found this PR. 👍

maxhutch
maxhutch reviewed Apr 13, 2020
View reviewed changes
Comment thread src/citrine/informatics/descriptors.py Outdated

def InorganicDescriptor(key: str, threshold: Optional[float] = 1.0):
"""[DEPRECATED] Use ChemicalFormulaDescriptor instead."""
logger.warning("InorganicDescriptor is deprecated and will soon be removed. "
Copy link
Copy Markdown
Contributor

@maxhutch maxhutch Apr 13, 2020

Choose a reason for hiding this comment

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

I believe the standard pattern for deprecations is to use the warnings package, e.g.:
https://github.com/CitrineInformatics/gemd-python/blob/master/taurus/__init__.py#L4
which was pattern-copied from a deprecated python standard module.

Copy link
Copy Markdown
Author

@bfolie bfolie Apr 13, 2020

Choose a reason for hiding this comment

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

changed

maxhutch
maxhutch approved these changes Apr 13, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@maxhutch maxhutch left a comment

Choose a reason for hiding this comment

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

LGTM

maxhutch
maxhutch suggested changes Apr 13, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@maxhutch maxhutch left a comment

Choose a reason for hiding this comment

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

Needs a version bump now

@bfolie bfolie requested a review from maxhutch April 13, 2020 21:12
maxhutch
maxhutch approved these changes Apr 13, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@maxhutch maxhutch left a comment

Choose a reason for hiding this comment

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

LGTM, again

@bfolie bfolie merged commit 7014c4c into master Apr 13, 2020
@bfolie bfolie deleted the PLA-3725/rename-inorganic-chemical-descriptor branch October 2, 2020 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@latture latture Awaiting requested review from latture

@Imperssonator Imperssonator Awaiting requested review from Imperssonator

1 more reviewer

@maxhutch maxhutch maxhutch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bfolie @andyczerwonka @maxhutch