-
Notifications
You must be signed in to change notification settings - Fork 5
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
make utils a subpackage of snmachine #259
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.
@heather999 I find it strange that I have no issues running the notebooks because in their imports they have
from snmachine import gps, sndata
from utils.plasticc_pipeline import create_folder_structure, get_directories, load_dataset
With this PR, I assumed that the second line had to be modified to
from snmachine.utils.plasticc_pipeline import ....
however, this modification returns an error and keeping the same does not.
Moreover, the tests didn't run for this PR and I have no way to re-run them, which is strange because for all other PRs I had no issues.
snmachine/snclassifier.py
Outdated
@@ -27,7 +27,7 @@ | |||
from sklearn import model_selection | |||
from sklearn.model_selection import PredefinedSplit, StratifiedKFold | |||
from sklearn.preprocessing import StandardScaler | |||
from utils import plasticc_utils | |||
from . utils import plasticc_utils |
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.
It seems that there is a space between .
and utils
that should not be here. However, I find no issues running the notebooks.
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 just updated this to from snmachine.utils
and that works for me - but it would be great if you could test it too
@heather999 did you have the time to look at my comments regarding this PR? |
@Catarina-Alves I think this is ready for testing by others. I did the following:
Does that work for you? |
Yes, thank you. I will now repeat the tests on my local machine. Once they pass, I will merge the PR and update the pip package. |
I had to update a few libraries in the setup to make all the tests run. I will now merge the PR, and update the version of |
IMPORTANT: Please create an issue first before opening a Pull Request.
Linked to issue(s): #258
Type of change
Please delete options that are not relevant.
What changes were proposed in this pull request?
Making the
utils
package a subpackage ofsnmachine
.How is the issue this PR is referenced against solved with this PR?
The
utils
directory has been moved undersnmachine
and corresponding code has been updated to referencesnmachine.utils
where appropriate.How was this patch tested?
Locally, I modified my copies of the example notebooks to use
snmachine.utils
and ran through the first 3 notebooks. It is necessary for someone to go through all the notebooks and rerun them to make sure things are ok.Final Checklist:
dev
branch ofsnmachine
at the time of this PR