problem compiling marketplace models (Trac #1043)

[wpotrzebowski]

When trying to compile models from marketplce numerous erros pop up related orientation/magnetism. This probably has something to do with recent orientation branch pushed to sasmodels. I've basically downloaded cylinder model from marketplace changed its name to marketplace_cylinder (also in python file where its pointing to c file) and run it from sasview as plugin model. One can probably do the same with sascomp.

Error message is long, so posting just a part of it

Traceback (most recent call last):
  File "/Users/wojciechpotrzebowski/SASVIEW_WORKSPACE/sasview/src/sas/sascalc/data_util/calcthread.py", line 274, in _run
    self.compute(*args, **kwargs)
RuntimeError: compile failed.
cc -shared -fPIC -std=c99 -O2 -Wall /var/folders/8c/pd135bx14f3_7xbhfxmkrcdx1ksm4c/T/sas64_marketplace_cylinder_qmYBBF.c -o /var/folders/8c/pd135bx14f3_7xbhfxmkrcdx1ksm4c/T/sas64_marketplace_cylinder.so -lm
/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:61:5: warning: implicit declaration of function 'ORIENT_SYMMETRIC' is invalid in C99 [-Wimplicit-function-declaration]
    ORIENT_SYMMETRIC(qx, qy, theta, phi, q, sin_alpha, cos_alpha);
    ^

/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:61:42: warning: variable 'q' is uninitialized when used here [-Wuninitialized]
    ORIENT_SYMMETRIC(qx, qy, theta, phi, q, sin_alpha, cos_alpha);
                                         ^
/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:60:13: note: initialize the variable 'q' to silence this warning
    double q, sin_alpha, cos_alpha;
            ^
             = 0.0
/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:61:45: warning: variable 'sin_alpha' is uninitialized when used here [-Wuninitialized]
    ORIENT_SYMMETRIC(qx, qy, theta, phi, q, sin_alpha, cos_alpha);
                                            ^~~~~~~~~
/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:60:24: note: initialize the variable 'sin_alpha' to silence this warning
    double q, sin_alpha, cos_alpha;
                       ^
                        = 0.0
/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:61:56: warning: variable 'cos_alpha' is uninitialized when used here [-Wuninitialized]
    ORIENT_SYMMETRIC(qx, qy, theta, phi, q, sin_alpha, cos_alpha);
                                                       ^~~~~~~~~
/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:60:35: note: initialize the variable 'cos_alpha' to silence this warning
    double q, sin_alpha, cos_alpha;
                                  ^
                                   = 0.0

It probably also applies to old plugin models. It also seems that dummy Iqxy shouldn't be included in models as they fail to compile too.
We probably need to provide a patch for old/marketplace models.

Migrated from http://trac.sasview.org/ticket/1043

{
    "status": "closed",
    "changetime": "2018-01-16T23:34:25",
    "_ts": "2018-01-16 23:34:25.388755+00:00",
    "description": "When trying to compile models from marketplce numerous erros pop up related orientation/magnetism. This probably has something to do with recent orientation branch pushed to sasmodels. I've basically downloaded cylinder model from marketplace changed its name to marketplace_cylinder (also in python file where its pointing to c file) and run it from sasview as plugin model. One can probably do the same with sascomp. \n\nError message is long, so posting just a part of it\n\n{{{\nTraceback (most recent call last):\n  File \"/Users/wojciechpotrzebowski/SASVIEW_WORKSPACE/sasview/src/sas/sascalc/data_util/calcthread.py\", line 274, in _run\n    self.compute(*args, **kwargs)\nRuntimeError: compile failed.\ncc -shared -fPIC -std=c99 -O2 -Wall /var/folders/8c/pd135bx14f3_7xbhfxmkrcdx1ksm4c/T/sas64_marketplace_cylinder_qmYBBF.c -o /var/folders/8c/pd135bx14f3_7xbhfxmkrcdx1ksm4c/T/sas64_marketplace_cylinder.so -lm\n/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:61:5: warning: implicit declaration of function 'ORIENT_SYMMETRIC' is invalid in C99 [-Wimplicit-function-declaration]\n    ORIENT_SYMMETRIC(qx, qy, theta, phi, q, sin_alpha, cos_alpha);\n    ^\n\n/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:61:42: warning: variable 'q' is uninitialized when used here [-Wuninitialized]\n    ORIENT_SYMMETRIC(qx, qy, theta, phi, q, sin_alpha, cos_alpha);\n                                         ^\n/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:60:13: note: initialize the variable 'q' to silence this warning\n    double q, sin_alpha, cos_alpha;\n            ^\n             = 0.0\n/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:61:45: warning: variable 'sin_alpha' is uninitialized when used here [-Wuninitialized]\n    ORIENT_SYMMETRIC(qx, qy, theta, phi, q, sin_alpha, cos_alpha);\n                                            ^~~~~~~~~\n/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:60:24: note: initialize the variable 'sin_alpha' to silence this warning\n    double q, sin_alpha, cos_alpha;\n                       ^\n                        = 0.0\n/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:61:56: warning: variable 'cos_alpha' is uninitialized when used here [-Wuninitialized]\n    ORIENT_SYMMETRIC(qx, qy, theta, phi, q, sin_alpha, cos_alpha);\n                                                       ^~~~~~~~~\n/Users/wojciechpotrzebowski/.sasview/plugin_models/marketplace_cylinder.c:60:35: note: initialize the variable 'cos_alpha' to silence this warning\n    double q, sin_alpha, cos_alpha;\n                                  ^\n                                   = 0.0\n}}}\nIt probably also applies to old plugin models. It also seems that dummy Iqxy shouldn't be included in models as they fail to compile too. \nWe probably need to provide a patch for old/marketplace models.\n\n\n",
    "reporter": "wojciech",
    "cc": "",
    "resolution": "fixed",
    "workpackage": "SasView Bug Fixing",
    "time": "2017-12-07T09:17:14",
    "component": "sasmodels Markeplace",
    "summary": "problem compiling marketplace models",
    "priority": "blocker",
    "keywords": "",
    "milestone": "SasView 4.2.0",
    "owner": "GitHub <noreply@github.com>",
    "type": "defect"
}

[pkienzle]

Trac update at 2017/12/08 13:47:19: pkienzle commented:

Yes, this will be a problem during the transition to the new sasview.

I don't think you want to maintain both old and new ways of calling the kernel in the code.

Doing a correct code transformation automatically would be much too hard.

(1) Could strip out old style Iqxy in the new version:

check if source contains the string ORIENT_A?SYMMETRIC. If so, look for “Iqxy”, scan forward to the next “{“, set nesting_level = 1 and scan the text, adding 1 for each ‘{‘ and subtracting 1 for each ‘}’ until nesting_level is 0. Replace the substring with “return NAN;”.

Something like:

    if “ORIENT_” in source:
        index = source.find(“Iqxy”) + 4
        c = source[index]
        while c != ‘{‘: index += 1
        index += 1
        start, levels = index, 1
        while levels > 0:
            c = source[index]
            if c == ‘{‘: levels += 1
            elif c == ‘}’: levels -= 1
            index += 1
        source = source[:start] + “return NAN;” + source[index+1:]

2D models will be broken for this model, but arguably they were anyway. Maybe spit something out through logger.

This does not cover the case of the trivial Iqxy = Iq(sqrt(qxqx + qyqy)).

(2) Could change API for the new models to use Iqac or Iqabc instead of Iqxy. Then strip any Iqxy function from the source with code similar to the above.

(3) Could document that old-style Iqxy models no longer work and give instructions on updating them.

[butlerpd]

Trac update at 2017/12/09 21:28:19: butler commented:

I am confused here. At this point there is only a handful of models in the marketplace that are different from the built in models ... including the cylinder model mentioned here. These models are supposed to be automatically updated if changed in sasmodels so they should just work. Is the real problem that the script to update marketplace models is not working?

[smk78]

Trac update at 2017/12/09 21:58:47: smk78 commented:

No, Lewis put all the built-ins on the Marketplace, remember.

[butlerpd]

Trac update at 2017/12/09 22:16:31: butler commented:

That is not what he said or documented on the wiki at
[wiki:DevNotes/Processeses/MarketplaceDeployment]
and I believe Was tested at least once.

[butlerpd]

Trac update at 2017/12/10 17:57:06: butler commented:

Interestingly models have not in fact been automatically updating since sep 7, 2017 it seems. This may be related to ticket #12. I note that a python only model was successfully uploaded to the marketplace on Dec 5, 2017. Probably need to check the server log files and file permissions as noted by Lewis in his notes.

[wpotrzebowski]

Trac update at 2017/12/11 08:24:58: wojciech commented:

Sep 7 is when Lewis finished off upload script: c5d579a and it was probably last time it was triggered.
Checking logs is probably good starting point. Should we assaign omeone with utk.edu credentials to this ticket?

[RichardHeenan]

Trac update at 2017/12/12 12:12:29: richardh commented:

[ To further clarify Paul K's comment above, I include below extracts of emails exchanged with him after I noticed that he had changed the way 2d intensity is calculated, less arguments to Iqxy and no longer any call to orient_symmetric or orient_assymmetric,  as part of the massive http://trac.sasview.org/ticket/776 orientation_776 ticket.]

[ There seem to be three submitted marketplace models (2 in cylinder, 1 in ellipsoid) that are actually affected, and as Paul B says could be edited by hand by us. BUT we would then have a backwards comaptibility issue if older versions of sasview are still to be supported by the marketplace, so there would need to be old and new versions.]

[ This does raise a more general issue about the marketplace, how do we maintain backwards compatibility or flag which version of sasview][ (or at least for the current and previous saview releases?)][ a marketplace model will work with if we change the way the sasview kernel calculates things.]

[ The marketplace could tell users where to find source code for built in sasview models in either their install directories (which would at least mean they have a version that works for them) or else perhaps on github? - though this would not be as elegant as the current links to download the code for models.]

[ Richard]

[ From: Paul Kienzle []mailto:paul.kienzle@nist.gov]

Sent: 02 November 2017 16:51

To: Heenan, Richard (STFC,RAL,ISIS)

Subject: Re: when did orient_asymmetric go? 

No longer need the theta, phi, psi arguments in Iqxy.

  - Paul

 

From: Paul Kienzle [mailto:paul.kienzle@nist.gov]

Sent: 02 November 2017 15:55

To: Heenan, Richard (STFC,RAL,ISIS)

Subject: Re: when did orient_asymmetric go? PS

Just to be clear,  in updating all the models I noticed that they all had q*xhat or the equivalent, so rather than sending in four parameters (q, xhat, yhat, zhat), I decided to send in (qx, qy, qz), renamed (qa, qb, qc) to avoid confusion with the lab-space qx, qy coordinate system.  This also means that I don't have to compute |q| as part of the kernel.  The only kernels that actually needed |q| were the paracrystal kernels, which use it for the radius in the spherical form factor.

   - Paul

Hi Paul,

In early October I gave  some users in Korea a special version of core_shell_bicelle_elliptical (attached fyi only) but that no longer compiles (at least in orientation branch) as there is now no longer a call to orient_asymmetric in the 2d intensity.

Looking at the history on git for core_shell_bicelle_elliptical.c does not show when or how you removed the call and sorted the parameters. Was this done automatically or by some hard work?

[more likely I was not looking in the correct branch?]

Thanks,  Richard

 

[butlerpd]

Trac update at 2017/12/12 19:05:23:

• butler changed _comment0 from "As agreed at the Tuesday fortnightly meeting, the first part of the answer will be Fix data upload to marketplace (Trac #685) #2 given above. The issue with the marketplace not updating is now in ticket #1047 while the need to add a "flag" to the marketplace so that the version of the API supported is documented with each model is in ticket #1045. Finally the need to change the handful of custom models in the marketplace that use 2D to use the new API is documented in ticket #1046." to "1513105676362899"
• butler commented:

As agreed at the Tuesday fortnightly meeting, the first part of the answer will be option 2 given above. The issue with the marketplace not updating is now in ticket #15 while the need to add a "flag" to the marketplace so that the version of the API supported is documented with each model is in ticket #13. Finally the need to change the handful of custom models in the marketplace that use 2D to use the new API is documented in ticket #14.

[pkienzle]

Trac update at 2017/12/13 23:27:27: pkienzle commented:

Yet another issue: line.Iqxy returns (m*qx+b)*(m*qy+b), but the new interface no longer provides Iqxy.

It could be useful to have arbitrary 2D calculations without orientation parameters converting qx, qy to qa, qb, qc, for example, if there is a strange 2D background that somebody is trying to model.

We can fake it by including theta, phi, psi fixed at 0 and hidden so that Iqabc(qa, qb, qc, ...) is the same as Iqabc(qx, qy, 0, ...), but that is messy.

So, do we need to continue to support Iqxy functions?

[pkienzle]

Trac update at 2017/12/14 20:23:23: pkienzle commented:

[https://github.com/SasView/sasmodels/pull/57 PR 57] supports Iqabc and Iqxy.

[sasview-bot]

Trac update at 2018/01/16 23:34:25:

• GitHub noreply@github.com commented:

In changeset 924a1196ee9fb4427c5f4eb32a3f0b8655184576:

#!CommitTicketReference repository="sasmodels" revision="924a1196ee9fb4427c5f4eb32a3f0b8655184576"
Merge pull request http://trac.sasview.org/ticket/57 from SasView/ticket-1043

use Iqac/Iqabc for the new orientation interface but Iqxy for the old. Fixes #11

• GitHub noreply@github.com changed owner from "" to "GitHub noreply@github.com"
• GitHub noreply@github.com changed resolution from "" to "fixed"
• GitHub noreply@github.com changed status from "new" to "closed"