-
Notifications
You must be signed in to change notification settings - Fork 23
refactor: update sasview api for test_sas.py #126
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
Conversation
@sbillinge here are my initial edits to update our code to use the new sasmodel/sasdata api. These changes introduced some warnings that I don't quite understand. Now that the tests in
I'll get to this issue later. |
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.
Please see comments
@@ -22,7 +22,6 @@ | |||
|
|||
from diffpy.srfit.exceptions import ParseError | |||
from diffpy.srfit.fitbase.profileparser import ProfileParser | |||
from diffpy.srfit.sas.sasimport import sasimport |
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 deleted the import from this file as not used but I haven't deleted sasimport
yet because the sas
package in sasview
still exists, and we still need to import from it.
src/diffpy/srfit/sas/sasparser.py
Outdated
loader = Loader() | ||
|
||
# Convert Path object to string if needed |
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.
load
seems to expect a string now because it's calling .lower()
on filename
. In the source code, the traceback leads to the lookup()
function in sasdata.data_util.registry
, which calls:
path_lower = path.lower()
src/diffpy/srfit/sas/sasparser.py
Outdated
@@ -118,7 +122,16 @@ def parseFile(self, filename): | |||
self._meta["filename"] = filename | |||
self._meta["datainfo"] = data | |||
|
|||
self._banks.append([data.x, data.y, data.dx, data.dy]) | |||
# Handle case where loader returns a list of data objects | |||
if isinstance(data, list): |
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 loader returns a list
now. From source code:
def load(self, file_path_list: Union[List[Union[str, Path]], str, Path],
format: Optional[Union[List[str], str]] = None
) -> List[Union[Data1D, Data2D]]:
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.
we presumably don't need the conditional then. If it returns a list then just treat a list. We don't have to backwards compatible because we are only supporting recent versions of all dependencies.
model = EllipsoidModel() | ||
model.setParam("radius_a", prad) |
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 could not find any mention of the use of radius_a
and radius_b
in the documentation for sasview, even ones that dated back to version 4.x (the latest release is version 6.1.0). I can only assume that suitable replacements are radius_polar
and radius_equatorial
, which are the parameters that the ellipsoid model now uses.
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.
EllipsoidModel
must have a major and a minor axis, so these presumably refer to this? What are these axes called in EllipsoidModel
?
I looked into the source code for sas, and it seems like the
Should I look into ways to somehow calculate the effective radius manually? How else should I modify the source code that the tests are testing? Or should I just deal with this in another PR? |
this appears not to be deprecated, but it is not implemented. I will have to look at the test |
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.
good work. Progress is being made. Please can you see my comments?
news/fix-sasmodels.rst
Outdated
|
||
**Changed:** | ||
|
||
* Refactored code utilizing sasmodels to use the new sasview api. |
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.
please move to fixed. This is a bug fix not a change. Reserve changes for changes in behavior that a user might need to know about.
src/diffpy/srfit/sas/sasparser.py
Outdated
|
||
Loader = sasimport("sas.dataloader.loader").Loader | ||
Loader = ld.Loader |
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.
please could you make ld
more readable. maybe sas_dataloader
?
src/diffpy/srfit/sas/sasparser.py
Outdated
loader = Loader() | ||
|
||
# Convert Path object to string if needed | ||
if not isinstance(filename, str): |
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.
why not just data = loader.load(str(filename))
. I don't think we have to wrap this in a conditional.
src/diffpy/srfit/sas/sasparser.py
Outdated
@@ -118,7 +122,16 @@ def parseFile(self, filename): | |||
self._meta["filename"] = filename | |||
self._meta["datainfo"] = data | |||
|
|||
self._banks.append([data.x, data.y, data.dx, data.dy]) | |||
# Handle case where loader returns a list of data objects |
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.
remove comment. Just make you code as readable as possible.
src/diffpy/srfit/sas/sasparser.py
Outdated
self._banks.append([data.x, data.y, data.dx, data.dy]) | ||
# Handle case where loader returns a list of data objects | ||
if isinstance(data, list): | ||
# If it's a list, iterate through each data object |
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.
remove gratuitous comments
@@ -19,8 +19,10 @@ | |||
import numpy | |||
import pytest | |||
|
|||
# Use the updated SasView model API to load models |
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.
please remove comments
@@ -34,7 +36,7 @@ def testSphere(sas_available): | |||
pytest.skip("sas package not available") | |||
radius = 25 | |||
# Calculate sphere cf from SphereModel | |||
SphereModel = sasimport("sas.models.SphereModel").SphereModel | |||
SphereModel = _make_standard_model("sphere") |
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 a bit odd that we are importing a private function. Are we sure this is the way we are supposed to be using the API?
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'll keep looking into this
model = EllipsoidModel() | ||
model.setParam("radius_a", prad) |
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.
EllipsoidModel
must have a major and a minor axis, so these presumably refer to this? What are these axes called in EllipsoidModel
?
@sbillinge I'm sorry, I couldn't figure out why the tests aren't passing with the default tolerance for |
@zmx27 I had a look at the sas models test and it is hard to figure out why it is off so much. I suggest that we
There are a few other issues with the sasview integration (it is not working beyond python 3.11) and I am not sure who is using it, if anyone. So revisiting later seems like the best bet, I will take a look at the characteristic function test that are failing now..... |
Should I look into ways to somehow calculate the effective radius manually? How else should I modify the source code that the tests are testing? Or should I just deal with this in another PR? I don't see where this is being called. It seems that Let's give it one more go before we punt it to a later release, but we can do that if we have to |
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.
@zmx27 please see my inline comment.
Sorry, I don't think I ever gave you a full traceback of the error. Here it is:
From what I can tell from the documentation, it seems like the |
Good, we are mkaing progress. this is tests working exactly as they are supposed to work. Remember, tests test behavior if they are done right. Here a dependency changed and the tests started failing. The reason the dependency changed in this case is not because they don't want to offer the same thing (functions that calculate characteristic functions for spherical particles) but because they want to change how they do that. We don't care how they do that, we just want the characteristic function of a spherical model. So our job is to figure out how they are handling that now (and they may have changed their API, they have apparently) and adapt all our code that uses this to make it work again. This is a fun project and worth doing, but it could take some time. I suggest that we
Then we can safely move this off the current release milestone. Let's leave this PR open as it will be quite hlepful for that work later. but change the release milestone on the issue that this closes. |
…d characteristicfunctions.py
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.
please see my comments.
@@ -415,6 +415,13 @@ def __call__(self, r): | |||
# | |||
# We also have to make a q-spacing small enough to compute out to at | |||
# least the size of the signal. | |||
|
|||
# As of release 3.2.0, these are not working but we hope to have |
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.
we would like this comment to be the NotImplemented
error message. No user will see this comment.
I am not sure how it is used, but do we also want this behavior in __init__()
constructor? Or is there some reason we may want it instantiated but not called?
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.
There doesn't seem to be any issues with instantiating the class so we don't need this error message in the __init__
constructor. The problematic line that results in a NotImplemented
error is when the __call__
function calls
ed = 2 * self._model.calculate_ER()
The calculate_ER()
function is not called in the __init__
function.
@@ -32,8 +32,7 @@ | |||
|
|||
|
|||
def testSphere(sas_available): | |||
if not sas_available: | |||
pytest.skip("sas package not available") | |||
pytest.skip("calculate_ER() not available") |
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.
this is the wrong error message. I think the ER function is not the issue. Just say "sas characteristic functions not currently working. Remove skip when our code is refactored to use the latest sasview API" or sthg like that.
Also, let's keep the original skip but commented out. We will need it again when we implement the new code.
@@ -53,8 +52,7 @@ def testSphere(sas_available): | |||
|
|||
|
|||
def testSpheroid(sas_available): | |||
if not sas_available: | |||
pytest.skip("sas package not available") | |||
pytest.skip("calculate_ER() not available") |
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.
please see above and handle all the models in the same way.
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.
please see my comments and I can merge this.
@@ -415,6 +415,11 @@ def __call__(self, r): | |||
# | |||
# We also have to make a q-spacing small enough to compute out to at | |||
# least the size of the signal. | |||
raise NotImplementedError( | |||
"As of release 3.2.0, these are not working but we hope to have " |
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.
change "these" to "SAS characteristic functions"
news/fix-sasmodels.rst
Outdated
|
||
**Fixed:** | ||
|
||
* Refactored code utilizing sasmodels to use the new sasview api. |
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.
let's change this to "changed" as "temporarily removed support for SAS characteristic functions until we can migrate to the new sasview api"
@sbillinge ready for review |
Everything looks good. Is there a reason it is marked as a draft? Or is it ready to merge? |
…dels() into the test functions
@sbillinge sorry, I forgot to mark this PR as ready for review. Also, it seems like CI is failing because Edit: Turns out I forgot to add sasmodels as a dependency for some reason… I’ll add it when I get back. |
we don't want to add that as a dependency if nothing is working with sas. Just add a skip if sasmodels is not installed for now. |
@sbillinge ready for review. I ended up adding each sasmodel import statement inside the test functions because commenting them out would result in unknown variables in |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #126 +/- ##
==========================================
- Coverage 67.05% 66.60% -0.46%
==========================================
Files 25 25
Lines 3160 3171 +11
==========================================
- Hits 2119 2112 -7
- Misses 1041 1059 +18
🚀 New features to boost your workflow:
|
Thanks, that's a good fix. I thought about how to handle this later and we could, later, either require sasview etc. as dependencies, or we could do like matplotlib and have In general we want srfit to be extensible, so if someone comes up with a XANES module or a Raman module we could do joint XANES/PDF refinements etc. and so in the future this problem could get worse and worse and worse, which is why it might be nice to have ways of testing the extensions but not making the basic package more and more bloated. |
Addresses #100