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

Update and deprecate top-level exports #1107

Merged
merged 7 commits into from Mar 19, 2020
Merged

Conversation

kjun9
Copy link
Contributor

@kjun9 kjun9 commented Mar 18, 2020

This removes (with deprecation) several of the re-exported names previously available at the top level of the stellargraph hierarchy. Specifically:

before after
stellargraph.expected_calibration_error stellargraph.calibration.expected_calibration_error
stellargraph.plot_reliability_diagram stellargraph.calibration.plot_reliability_diagram
stellargraph.Ensemble stellargraph.ensemble.Ensemble
stellargraph.BaggingEnsemble stellargraph.ensemble.BaggingEnsemble
stellargraph.TemperatureCalibration stellargraph.calibration.TemperatureCalibration
stellargraph.IsotonicCalibration stellargraph.calibration.IsotonicCalibration

It also adds the remaining names to __all__, so that they're imported when one writes from stellargraph import * and thus that is consistent with the names that are accessible as stellargraph.foo (after import stellargraph).

See #714

@codeclimate
Copy link

codeclimate bot commented Mar 18, 2020

Code Climate has analyzed commit 711a6de and detected 4 issues on this pull request.

Here's the issue category breakdown:

Category Count
Style 4

View more on Code Climate.

@kjun9 kjun9 marked this pull request as ready for review March 18, 2020 06:24
Copy link
Member

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Looks good as is, but there's a question that may result in either follow up work or bigger changes here.

stellargraph/__init__.py Outdated Show resolved Hide resolved
stellargraph/__init__.py Show resolved Hide resolved
stellargraph/__init__.py Show resolved Hide resolved
stellargraph/__init__.py Show resolved Hide resolved
stellargraph/__init__.py Show resolved Hide resolved
@kjun9
Copy link
Contributor Author

kjun9 commented Mar 19, 2020

@huonw

I've tried removing the other top-level exports and added the deprecation warnings like you suggested (but this also suffers from #1117 I'm guessing). I kept the classes from core as top level exports since core is not part of the top-level __all__ at all.

@kjun9 kjun9 requested a review from huonw March 19, 2020 02:16
Copy link
Member

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Looks good!

Could you update the PR title (or at least, the final commit message) to reference that it's also deprecating some of our top-level exports?

(For these things that are just changing imports, it's probably ok to just commit the code change by itself, without the rest of the slightly-different-randomness changes from rerunning the notebooks... as long as the notebook runs, of course.)

stellargraph/__init__.py Outdated Show resolved Hide resolved
stellargraph/__init__.py Outdated Show resolved Hide resolved
@kjun9 kjun9 changed the title Add all top-level exports to __all__ Update and deprecate top-level exports Mar 19, 2020
@kjun9 kjun9 requested a review from huonw March 19, 2020 02:54
Copy link
Member

@huonw huonw left a comment

Choose a reason for hiding this comment

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

This looks good to me, and the warnings seem to appear in Jupyter at least 👍

It may be good to validate with @PantelisElinas, who added both the calibration (#326) and ensemble (#343) code.

@kjun9 kjun9 requested review from PantelisElinas and removed request for kieranricardo March 19, 2020 03:35
@kjun9
Copy link
Contributor Author

kjun9 commented Mar 19, 2020

Hi @PantelisElinas I've added you as reviewer - for now this can wait in case you're available soon to check my assumption that the ensemble and calibration functions/classes don't need to be re-exported at the top-level, but I'll also eventually just go ahead with the merge if that's not the case.

Copy link
Contributor

@PantelisElinas PantelisElinas left a comment

Choose a reason for hiding this comment

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

It looks ok to me, if this is the way it should be.

P.

@kjun9
Copy link
Contributor Author

kjun9 commented Mar 19, 2020

if this is the way it should be.

Classes and functions in ensemble and calibration didn't particularly occur to me as something to be treated differently from things in other modules, so that's the motivation for removing them from the top-level so that we can be consistent now and in future, but not that there's a way it should be if that makes sense.

Thanks for taking a look!

@kjun9 kjun9 merged commit 3348ad4 into develop Mar 19, 2020
@kjun9 kjun9 deleted the bugfix/714-top-level-exports branch March 19, 2020 23: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.

None yet

3 participants