-
Notifications
You must be signed in to change notification settings - Fork 20
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
removing Pyne import when not required #184
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @bam241!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found a few more places, since we're doing this here.
cymetric/graphs.py
Outdated
import pandas as pd | ||
import numpy as np |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need these here, either.
cymetric/graphs.py
Outdated
@@ -13,7 +14,6 @@ | |||
|
|||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this needs any of PyNE?
Not an issue with me, I am actually glad some of my old work could be useful! |
This is now failing tests, but that may be because of upstream changes... Let's wait until the CI refactor is done and take another look |
I've merged in the main branch, which includes the CI work @bennibbelink did. It looks like some of the tests still aren't passing, so I'll work on this. |
It looks like it has trouble building Cymetric, and the error is
Is this something with the new CI work you did @bennibbelink? |
@abachma2 Yes it is. Cymetric is building successfully but the workflow is attempting to push an image (the layer cache) to the cyclus GHCR. Since this PR is coming from a fork it doesn't have permission. This is a flaw in the new CI strategy, I will try to find a solution quickly |
That makes a lot of sense. If you can't figure out a way to allow this, we should really add a note to all of the contributing documentation for the Cyclus, Cycamore, and Cymetric repos that PRs need to come from the cyclus remote to allow running the tests. |
I see two ways to resolve the 403:
The second solution should be really easy to implement and fix the issue. I'll go ahead and make a PR so we can get this resolved quickly |
This exposes another issue in the CI refactor - the |
I would agree that the second method is preferred. Thanks for taking care of all of this. |
Build Status ReportBuild |
I'm confident this is all OK based on this testing, and we'll figure out the upstream/downstream testing as we go. |
removing import that are not used when importing PyNE.
useful when those modules are neither build nor required :)