raise `ValueError` if the the number of regression points is less than the number of parameters in `_residual` #249

ycexiao · 2025-08-18T20:54:57Z

What problem does this PR address?

Address the second item in #241.

What should the reviewer(s) do?

Please check the modifications.

…n the number of parameters in `_residual`

codecov · 2025-08-18T20:57:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.92%. Comparing base (6fdc0af) to head (daf51c3).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #249      +/-   ##
==========================================
+ Coverage   99.22%   99.92%   +0.70%     
==========================================
  Files          23       23              
  Lines        1285     1318      +33     
==========================================
+ Hits         1275     1317      +42     
+ Misses         10        1       -9

Files with missing lines	Coverage Δ
tests/test_refine.py	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ycexiao · 2025-08-18T20:57:34Z

@sbillinge, it's ready for review.

Sparks29032 · 2025-08-20T01:35:03Z

Make sure that the argument parser captures the error in morphapp.py. Since the parser works on command line, you want to use its inbuilt error function (though I believe there is also a custom error function). The end result on the CLI side should be to throw exit 1 rather than a Python error, but the Python side should be fine now. The user will not see the error on CLI if there is no exit.

I would also like it if there was a test that this error is indeed being captured? Maybe one test for the CLI side and one for the morphpy side?

sbillinge · 2025-08-20T05:00:50Z

@ycexiao @Sparks29032 I don't really mind where the error is caught in python or in CLI as long as the help message helps the user as much as possible to come up with a fix. Saying "adjust somethihng" is not so helpful because what do I adjust it to? or how do I figure out what to adjust it to? @Sparks29032 is there a way to know and suggest what adjustment will fix this? If it is trial and error to try and fix it, or the user can plot the polynomial and see where it goes negative or some such thing, this is helpful information to help them debug. We don't want to write a book, but some brief and hdlpful explanation would be good.

Yes, we need a test for this. If we implement it as a check in the CLI we could test that there too. As a rule we have not been doing much validation at all in the CLI. The main reason is just not enough time in the day. So I would not really prioritize that that atm unless it is quick and painless.

Sparks29032 · 2025-08-20T05:04:19Z

What I am saying is that the error is not going to display to a CLI user if we do not exit. We want a wrapper around the refine call in the morphapp.py file that checks if we have a Python error and throws an exit.

sbillinge · 2025-08-20T10:31:24Z

aren't we raising an exception? Are saying the exception gets caught and handled somewhere higher up in the code stack? If not then it will crash with the exception (which would be the desired behavior)?

ycexiao · 2025-08-20T15:31:40Z

Saying "adjust something" is not so helpful because what do I adjust it to? or how do I figure out what to adjust it to?

How about "Please make sure the overlapping domain between the morphed function and the target function is sufficiently large, or reduce the number of parameters?"

aren't we raising an exception? Are you saying the exception gets caught and handled somewhere higher up in the code stack? If not, then it will crash with the exception (which would be the desired behavior)?

I will make sure both the CLI user and the scripts user will see the error message.

I would also like it if there were a test that this error is indeed being captured? Maybe one test for the CLI side and one for the Morphpy side?

Yes, I will add a test for it.

Sparks29032 · 2025-08-20T15:38:04Z

Sounds good for the adjustment. Would also be helpful to mention the size of the function and number of parameters you are optimizing to the user so they can see exactly how many more parameters they must reduce by.

ycexiao

@sbillinge @Sparks29032, it's ready for review.

ycexiao · 2025-08-20T22:53:37Z

src/diffpy/morph/refine.py

-
+        if len(rvec) < len(pvals):
+            raise ValueError(
+                f"\nNumber of shared grid points: {len(rvec)}\n"


Print the size of the function and the number of parameters.

tests/test_refine.py

sbillinge

nice work everyone

sbillinge · 2025-08-19T13:51:34Z

src/diffpy/morph/refine.py

+            raise ValueError(
+                "Not enough shared grid points between morphed function "
+                "and target function to fit the chosen parameters. "
+                "Please adjust the functions or "


Nicely done. Is there a way to give guidance on how the to "adjust the functions"?

The current error message is: "Please make sure the overlapping domain between the morphed function and the target function is sufficiently large, or reduce the number of parameters."

sbillinge · 2025-08-21T01:51:09Z

src/diffpy/morph/refine.py

                "Not enough shared grid points between morphed function "
-                "and target function to fit the chosen parameters. "
-                "Please adjust the functions or "
+                "between morphed function and target function to fit "


please check, is "between morphed function" repeated here?

Yes, it is a typo. It is fixed in a newer commit.

src/diffpy/morph/refine.py

ycexiao

@sbillinge @Sparks29032, it's ready for review.

Sparks29032 · 2025-08-21T22:52:31Z

I'm not sure the following lines are being run in the test (see CodeCov).

actual_error_message = str(error.value)
assert actual_error_message == expected_error_message

When the error is thrown and Pytest catches it, the lines below where the error is thrown are not run.

You will need some other way to check the error message is correct.

Otherwise, I have no other comments, seems good.

This reverts commit 91fa347.

ycexiao

@sbillinge @Sparks29032, it's ready for review.

ycexiao · 2025-08-22T03:38:11Z

tests/test_refine.py

+            ValueError,
+        ) as error:
+            refiner.refine(*refpars)
+        actual_error_message = str(error.value)


These two lines were skipped because of the wrong indentation. It is fixed now.

sbillinge

I am not sure I like this. Why are we doing this? Lot's of extra complicated code to maintain, but what benefit does it get us exactly? Let's discuss a bit. I am happy to be convinced, but in general, simpler is better for a research group like ours.....

Sparks29032 · 2025-08-22T16:59:36Z

Two main reasons why I'd like this PR:
(1) When this error occurs, a quite unhelpful leastsq error message pops up and it is not too clear what one needs to fix in the case that the user really has an exorbitant number of parameters say in their funcxy function.
(2) When I ran into this error, it was because I was reading the transpose of the two-column array lists. In other words, rather than getting 4000 grid points, I am getting 1. Seeing this error and letting me know that my grid overlap is some absurdly small number like 1 or 0 lets me know that the error has to do with my data preprocessing/obtaining, not some weird regressor fault, which a random leastsq error message may imply to a user who is not too familiar w/ debugging.

ycexiao · 2025-08-22T17:07:14Z

I am not sure I like this. Why are we doing this? Lot's of extra complicated code to maintain, but what benefit does it get us exactly? Let's discuss a bit. I am happy to be convinced, but in general, simpler is better for a research group like ours.....

@sbillinge, are you referring to the commit "use a custom error to stop leatsq"? Actually, it is not a good fix and it is reverted. The actual modification is from chore: fix the code indentation. where only the code indentation is changed so it is not complicated.

sbillinge · 2025-08-23T01:24:51Z

ok, sounds good all around

raise ValueError if the the number of regression points is less tha…

bc24486

…n the number of parameters in `_residual`

ycexiao marked this pull request as ready for review August 18, 2025 20:57

Sparks29032 mentioned this pull request Aug 20, 2025

raise ValueError if x_squeezed is not strictly increasing #248

Closed

ycexiao added 2 commits August 20, 2025 18:48

test: add test for not enough shared grid points the bad case.

f537512

chore: fix typo

9832900

ycexiao commented Aug 20, 2025

View reviewed changes

sbillinge reviewed Aug 21, 2025

View reviewed changes

ycexiao added 2 commits August 21, 2025 18:16

chore: more concise error message

71f10e5

chore: fix pre-commit

935dd8e

ycexiao commented Aug 21, 2025

View reviewed changes

src/diffpy/morph/refine.py Show resolved Hide resolved

ycexiao commented Aug 21, 2025

View reviewed changes

ycexiao added 3 commits August 21, 2025 23:24

fix: use a custom error to stop leatsq

91fa347

Revert "fix: use a custom error to stop leatsq"

5b0f30b

This reverts commit 91fa347.

chore: fix the code indentation.

daf51c3

ycexiao commented Aug 22, 2025

View reviewed changes

sbillinge reviewed Aug 22, 2025

View reviewed changes

sbillinge merged commit 3908d63 into diffpy:main Aug 23, 2025
5 checks passed

Sparks29032 mentioned this pull request Sep 2, 2025

Remove subprocess calls from tests #251

Merged

raise ValueError if the the number of regression points is less than the number of parameters in _residual #249

raise ValueError if the the number of regression points is less than the number of parameters in _residual #249

Uh oh!

Conversation

ycexiao commented Aug 18, 2025

What problem does this PR address?

What should the reviewer(s) do?

Uh oh!

codecov bot commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ycexiao commented Aug 18, 2025

Uh oh!

Sparks29032 commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbillinge commented Aug 20, 2025

Uh oh!

Sparks29032 commented Aug 20, 2025

Uh oh!

sbillinge commented Aug 20, 2025

Uh oh!

ycexiao commented Aug 20, 2025

Uh oh!

Sparks29032 commented Aug 20, 2025

Uh oh!

ycexiao left a comment

Choose a reason for hiding this comment

Uh oh!

ycexiao Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

sbillinge Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

ycexiao Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

sbillinge Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

ycexiao Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ycexiao left a comment

Choose a reason for hiding this comment

Uh oh!

Sparks29032 commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycexiao left a comment

Choose a reason for hiding this comment

Uh oh!

ycexiao Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Sparks29032 commented Aug 22, 2025

Uh oh!

ycexiao commented Aug 22, 2025

Uh oh!

Uh oh!

sbillinge commented Aug 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

raise `ValueError` if the the number of regression points is less than the number of parameters in `_residual` #249

raise `ValueError` if the the number of regression points is less than the number of parameters in `_residual` #249

codecov bot commented Aug 18, 2025 •

edited

Loading

Sparks29032 commented Aug 20, 2025 •

edited

Loading

Sparks29032 commented Aug 21, 2025 •

edited

Loading