enabled use of different independent_vars...

[Julian-Hochhaus]

... in Composite Model
The independent_vars in the CompositeModel are now no longer only the independent_vars of the left model.
Now, the independent_vars of the left model and right model are both passed to the independent_vars of the CompositeModel.
To reach that, I simply changed l.1120 in model.py from:

if 'independent_vars' not in kws:

    kws['independent_vars'] = self.left.independent_vars

to :

if 'independent_vars' not in kws:

    kws['independent_vars'] = list(dict.fromkeys([*self.left.independent_vars, *self.right.independent_vars]))

This fix solves the problem described in detail here.

Type of Changes

• Bug fix
• New feature
• Refactoring / maintenance
• Documentation / examples

Tested on

Python: 3.8.3 (default, Jul 2 2020, 16:21:59)
[GCC 7.3.0]

lmfit: 1.0.3.post31+gb930dde.d20220613, scipy: 1.5.0, numpy: 1.18.5, asteval: 0.9.26, uncertainties: 3.1.6

Verification

Have you

• included docstrings that follow PEP 257? --> only small modification in Docstring (CompositeModel--> NOTES)
• referenced existing Issue and/or provided relevant link to mailing list?
• verified that existing tests pass locally?
• verified that the documentation builds locally?
• squashed/minimized your commits and written descriptive commit messages?
• added or updated existing tests to cover the changes?
• updated the documentation and/or added an entry to the release notes (doc/whatsnew.rst)?
• added an example?

[newville]

@Julian-Hochhaus @reneeotten I'm not certain why this is failing... Anyone have any idea?

[reneeotten]

@newville @Julian-Hochhaus Part of it is an old failure that I haven't been able to figure out. In a few tests the number of func evaluation doesn't appear to be correct for "certain Python versions with latest dependencies, bit only on Linux".

New failures since this PR are due yo Sphinx version 5. Some stuff has changed there and now throws a warning (which we deliberately treat as an error). Nothing too problematic I'm sure - just haven't found the time yet to look.

We should really fix the new issues and find a way around the "old" ones so we can actually use the test-suite again to check new code. I'll try to do that this weekend, things have been a bit hectic after just changing jobs.

[reneeotten]

The functional change looks fine I think. But first making a dictionary and then converting it to a list seems a bit awkward to me. Is this the easiest/cleanest way of doing it (I haven't checked myself yet)?

[newville]

perhaps

       ivar1, ivar2 =  self.left.independent_vars,  self.right.independent_vars
       kws['independent_vars'] = ivar1 + [x for x in ivar2 if x not in ivar1] 

[Julian-Hochhaus]

I have reviewed my solution and the alternative suggestions by @newville. Performance-wise the solution of making a dictionary and later converting it back to a list is generally faster. If the lengths of lists of independent_vars are in the range of n=100 by a factor of 30, for shorter and more reasonable lengths of n=10 it is only a factor of 2.

Obviously, in typical cases, the execution speed will never play a role because we will probably never have large numbers of independent variables. I will therefore use a version similar to @newville 's one, for better readability.

[reneeotten]

@Julian-Hochhaus also please add a test (e.g., based on the code you used to show the issue) that failed before and is now fixed with this PR.

[newville]

@reneeotten OK, thanks. It looks like some of the failures might be due to high expected precisions and highly precise expected number of function evaluations. I think some of those tolerances could probably be relaxed...
But, maybe I do not fully understand the Azure framework well enough to be sure.

[reneeotten]

@Julian-Hochhaus can you please rebase with master and push again to this PR so that the tests will be triggered again? I've fixed the test failures that were not related to this PR but we do want to make sure that your suggested changes don't break something elsewhere. Thanks!

[Julian-Hochhaus]

@reneeotten @newville I was on vacation for the last two weeks, I will review the proposed optimization of the code and will rebase and trigger the tests again as soon as possible.

[Julian-Hochhaus]

@Julian-Hochhaus also please add a test (e.g., based on the code you used to show the issue) that failed before and is now fixed with this PR.

I surely will asap and will clean my commit history too, so that the pull request, later on, gets nice and tidy

[Julian-Hochhaus]

@reneeotten I have created an example that shows that the code now works with two models with different independant_vars.
However, it is my first time creating such a test file, and after looking into other test files in this repo, I am a bit confused regarding the format and functionality that my test file will need. However, I am willing to learn that, so any help is welcome, especially regarding the functionality and principal structure that my test file may need.
Kind regards

[newville]

@Julian-Hochhaus I'll admit to being hesitant about any analysis involving Azure, but it looks like maybe you need to remove the import matplotlib... from your added test. The test is not going to do any plotting.

As an aside (or maybe a rant ;) ) import matplotlib.pyplot as plt is just a terrible "throwaway line" to add reflexively to Python programs. Pyplot will make bold and unpredictable assertions about windowing environment for the script running it (like, can the application that is this script draw to the current definition of "the screen", and if so, using which drawing libraries would it use?). For "plain, simple scripts", Jupyter notebooks, and the sort of 20-line Python scripts, it's a very common thing to add:

    import numpy as np    # <- yes, no problem
    import pandas as pd   # <- hm, maybe
    import matplotlib.pyplot as plt # <- the yuck.

and it's probably fine for, you know, all the simple cases. We're as guilty as the next person - it is in many of our examples.
Bu If anyone tells you that "it works fine", you can complete their sentence with "... until it fails catastrophically". When embedded in a library or any kind of code that might be imported, that should be, at the very least:

    import numpy as np    # <- yes, no problem
    import pandas as pd 
    if __name__ == '__main__`:
          import matplotlib.pyplot as plt

In any production library or testing code, it is certain to cause failures.

Basically, matplotlib is great, and matplotlib.pyplot is evil.

Thanks for allowing the rant... ;)

[Julian-Hochhaus]

@newville Thanks for the rant :). I have removed the plotting because it was not necessary to show the improvement of the code via plotting.

I think the pull request is therefore finally ready to be published.

[reneeotten]

I’ll take a look during the holiday weekend

[newville]

Thanks, Looks good to me! +1 on merging.

[reneeotten]

@Julian-Hochhaus @newville I still see a few issues with this PR.

1. the test you added is actually not run with pytest since you didn't name the function correctly (i.e., the function definition should start with test).
2. more importantly though the test is actually not a regression test in the sense that it already "works" (or at least the test doesn't fail) with the current version of lmfit. What we want is a test that fails right now and after your proposed fix it should pass; that will make sure that we never reintroduce the issue at some later stage.

In short, I would recommend to add a test in test_model.py; you shouldn't don't need all the data and stuff, it should suffice to make define two models with different independent_vars and make a CompositeModel out of that. With the current version of lmfit that should reproduce the trouble you're seeing, after the change it should pass.

[reneeotten]

how about
kws['independent_vars'] = np.unique(self.left.independent_vars + self.right.independent_vars)
?

[reneeotten]

sorry, this was my bad... it should return a list not an array so that conversion is required here

[Julian-Hochhaus]

Sorry to annoy you again, but I may need your help again @reneeotten. I am not sure why it is failing right now, assume it has to do with the testing in test_model.py

[reneeotten]

@Julian-Hochhaus okay, I think I've fixed up the things that needed changes...

For future reference: (1) it's easier to make changes in a separate branch than master, (2) please do not make unnecessary whitespace changes you did in test_model.py, I reverted all of those, (3) as I said before regression test should fail with the current version and pass after the changes you made, that wasn't the case with the test you added. Please check the changes in that I made in the test - I think this captures what you intended to test for and indeed failed before this PR.

Some of these things, amongst others are covered in our CONTRIBUTING guidelines.

[newville]

@Julian-Hochhaus @reneeotten Thanks to both of you for seeing this through!

[Julian-Hochhaus]

@reneeotten Thanks a lot for your help, I learned so much on the way. It was my first public pull request :).

Thanks for your remarks, they will help me a lot in the future while contributing to public projects!