-
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
Package upgrades #159
Package upgrades #159
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #159 +/- ##
===========================================
- Coverage 89.03% 76.26% -12.77%
===========================================
Files 23 23
Lines 1395 1403 +8
===========================================
- Hits 1242 1070 -172
- Misses 153 333 +180 ☔ View full report in Codecov by Sentry. |
deprecate ipywe module [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci scipy upgrades, imp module update
b444ace
to
166eaa9
Compare
https://github.com/neutrons/multiphonon/blob/package_upgrades/tests/redutils_TestCase.py#L4 has |
tests/getdos_TestCase2.py
Outdated
dataurls = imp.load_source("dataurls", os.path.join(datadir, "dataurls.py")) | ||
|
||
|
||
def load_source(modname, filename): |
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.
the load_source function looks the same as above. It can be included in one spot and imported everywhere.
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.
These have been fixed. The load_source function is now in multiphonon.sqe.init.py
there are various warnings, when running pytest: "/multiphonon/backward/stitch_dos.py:100: UserWarning: Scaling factor to combine DOSes calculated is not stable:" Is this expected? |
What about this one ?: |
@@ -1,4 +1,8 @@ | |||
# -*- python -*- | |||
# This algorithm should be deprecated with the deletion of ipywe. This algorithm is not covered in tests. |
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.
Can you include in your PR summary how to import this to have it as a reference, please?
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.
Yes, there is an added instructions now
The warnings are expected. The matplotlib depreactation warning may produce errors in the future. A new ticket should be created to capture it or address when it happens. |
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 looks good to me!
Short description of the changes:
Upgraded every packages to latest version except pyre.
Fixed the deprecated interp2d function in scipy.
Implemented a custom function to upgrade imp module
Made sure python version is > 3.10
Kept getdos method with upgraded scipy version
deprecated ui/getdos.py since this is not in tests and once ipywe is remvoed this is no longer functioning
Removed ipywe and removed related tests that import ipywe.
ui/getdos.py is deprecated (renamed to ui/getdos_deprecated.py). This algorithm fails with the deletion of ipywe. This algorithm is also not covered in tests. Just in case users are using old jupyternotebooks that calls this algorithm, first,
pip install ipywe
thenimport multiphonon.ui.getdos_deprecated as (name)
.Story 6599
Long description of the changes:
Check list for the pull request
Check list for the reviewer
Manual test for the reviewer
To test, git pull this branch, run pytest make sure all tests pass. Check version numbers for numpy, scipy, python, mantid, matplotlib should all be latest version
References